| Message ID | 20220909034611.390710-1-philipl@overt.org |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavu/pixdesc: favour formats where bit depth exactly matches | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Thu, 2022-09-08 at 20:46 -0700, Philip Langdale wrote: > Since introducing the various packed formats used by VAAPI (and p012), > we've noticed that there's actually a gap in how > av_find_best_pix_fmt_of_2 works. It doesn't actually assign any value > to having the same bit depth as the source format, when comparing > against formats with a higher bit depth. This usually doesn't matter, > because av_get_padded_bits_per_pixel() will account for it. > > However, as many of these formats use padding internally, we find that > av_get_padded_bits_per_pixel() actually returns the same value for the > 10 bit, 12, bit, 16 bit flavours, etc. In these tied situations, we end 12 bit, > up just picking the first of the two provided formats, even if the > second one should be preferred because it matches the actual bit depth. > > This bug already existed if you tried to compare yuv420p10 against p016 > and p010, for example, but it simply hadn't come up before so we never > noticed. > > But now, we actually got a situation in the VAAPI VP9 decoder where it > offers both p010 and p012 because Profile 3 could be either depth and > ends up picking p012 for 10 bit content due to the ordering of the > testing. > > In addition, in the process of testing the fix, I realised we have the > same gap when it comes to chroma subsampling - we do not favour a > format that has exactly the same subsampling vs one with less > subsampling when all else is equal. > > To fix this, I'm introducing a small score penalty if the bit depth or > subsampling doesn't exactly match the source format. This will break > the tie in favour of the format with the exact match, but not offset > any of the other scoring penalties we already have. > > I have added a set of tests around these formats which will fail > without this fix. > > Signed-off-by: Philip Langdale <philipl@overt.org> > --- > libavutil/pixdesc.c | 16 +++++- > libavutil/tests/pixfmt_best.c | 93 ++++++++++++++++++++++++++++------- > tests/ref/fate/pixfmt_best | 2 +- > 3 files changed, 89 insertions(+), 22 deletions(-) > > diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c > index d7c6ebfdc4..412e257a30 100644 > --- a/libavutil/pixdesc.c > +++ b/libavutil/pixdesc.c > @@ -3004,6 +3004,11 @@ static int get_pix_fmt_score(enum AVPixelFormat > dst_pix_fmt, > if ((ret = get_pix_fmt_depth(&dst_min_depth, &dst_max_depth, > dst_pix_fmt)) < 0) > return -3; > > + // Favour formats where bit depth exactly matches. If all other scoring > is > + // equal, we'd rather use a lower bit depth that matches the source. > + if (src_min_depth != dst_min_depth || src_max_depth != dst_max_depth) > + score -= 64; > + > src_color = get_color_type(src_desc); > dst_color = get_color_type(dst_desc); > if (dst_pix_fmt == AV_PIX_FMT_PAL8) > @@ -3020,14 +3025,21 @@ static int get_pix_fmt_score(enum AVPixelFormat > dst_pix_fmt, > } > > if (consider & FF_LOSS_RESOLUTION) { > + // Apply a large penalty if there's a loss of resolution, but also > apply > + // a small penalty of the dst resolution is higher so that we favour > + // exactly matching formats. > if (dst_desc->log2_chroma_w > src_desc->log2_chroma_w) { > loss |= FF_LOSS_RESOLUTION; > score -= 256 << dst_desc->log2_chroma_w; > - } > + } else if (dst_desc->log2_chroma_w < src_desc->log2_chroma_w) > + score -= 32; > + > if (dst_desc->log2_chroma_h > src_desc->log2_chroma_h) { > loss |= FF_LOSS_RESOLUTION; > score -= 256 << dst_desc->log2_chroma_h; > - } > + } else if (dst_desc->log2_chroma_h < src_desc->log2_chroma_h) > + score -= 32; > + > // don't favor 422 over 420 if downsampling is needed, because 420 > has much better support on the decoder side > if (dst_desc->log2_chroma_w == 1 && src_desc->log2_chroma_w == 0 && > dst_desc->log2_chroma_h == 1 && src_desc->log2_chroma_h == 0 ) { > diff --git a/libavutil/tests/pixfmt_best.c b/libavutil/tests/pixfmt_best.c > index 0542af494f..b5d4d04976 100644 > --- a/libavutil/tests/pixfmt_best.c > +++ b/libavutil/tests/pixfmt_best.c > @@ -39,32 +39,59 @@ static const enum AVPixelFormat pixfmt_list[] = { > AV_PIX_FMT_VAAPI, > }; > > -static enum AVPixelFormat find_best(enum AVPixelFormat pixfmt) > +static const enum AVPixelFormat semiplanar_list[] = { > + AV_PIX_FMT_P016, > + AV_PIX_FMT_P012, > + AV_PIX_FMT_P010, > + AV_PIX_FMT_NV12, > +}; > + > +static const enum AVPixelFormat packed_list[] = { > + AV_PIX_FMT_XV36, > + AV_PIX_FMT_XV30, > + AV_PIX_FMT_VUYX, > + AV_PIX_FMT_Y212, > + AV_PIX_FMT_Y210, > + AV_PIX_FMT_YUYV422, > +}; > + > +typedef enum AVPixelFormat (*find_best_t)(enum AVPixelFormat pixfmt); > + > +#define find_best_wrapper(name, list) \ > +static enum AVPixelFormat find_best_ ## name (enum AVPixelFormat pixfmt) \ > +{ \ > + enum AVPixelFormat best = AV_PIX_FMT_NONE; \ > + int i; \ > + for (i = 0; i < FF_ARRAY_ELEMS(list); i++) \ > + best = av_find_best_pix_fmt_of_2(best, list[i], \ > + pixfmt, 0, NULL); \ > + return best; \ > +} > + > +find_best_wrapper(base, pixfmt_list) > +find_best_wrapper(seminplanar, semiplanar_list) > +find_best_wrapper(packed, packed_list) > + > +static void test(enum AVPixelFormat input, enum AVPixelFormat expected, > + int *pass, int *fail, find_best_t find_best_fn) > { > - enum AVPixelFormat best = AV_PIX_FMT_NONE; > - int i; > - for (i = 0; i < FF_ARRAY_ELEMS(pixfmt_list); i++) > - best = av_find_best_pix_fmt_of_2(best, pixfmt_list[i], > - pixfmt, 0, NULL); > - return best; > + enum AVPixelFormat output = find_best_fn(input); > + if (output != expected) { > + printf("Matching %s: got %s, expected %s\n", > + av_get_pix_fmt_name(input), > + av_get_pix_fmt_name(output), > + av_get_pix_fmt_name(expected)); > + ++(*fail); > + } else > + ++(*pass); > } > > int main(void) > { > - enum AVPixelFormat output; > int i, pass = 0, fail = 0; > > -#define TEST(input, expected) do { \ > - output = find_best(input); \ > - if (output != expected) { \ > - printf("Matching %s: got %s, expected %s\n", \ > - av_get_pix_fmt_name(input), \ > - av_get_pix_fmt_name(output), \ > - av_get_pix_fmt_name(expected)); \ > - ++fail; \ > - } else \ > - ++pass; \ > - } while (0) > +#define TEST(input, expected) \ > + test(input, expected, &pass, &fail, find_best_base); > > // Same formats. > for (i = 0; i < FF_ARRAY_ELEMS(pixfmt_list); i++) > @@ -137,6 +164,34 @@ int main(void) > // Opaque formats are least unlike each other. > TEST(AV_PIX_FMT_DXVA2_VLD, AV_PIX_FMT_VDPAU); > > +#define TEST_SEMIPLANAR(input, expected) \ > + test(input, expected, &pass, &fail, find_best_seminplanar); > + > + // Same formats. > + for (i = 0; i < FF_ARRAY_ELEMS(semiplanar_list); i++) > + TEST_SEMIPLANAR(semiplanar_list[i], semiplanar_list[i]); > + > + // Formats containing the same data in different layouts. > + TEST_SEMIPLANAR(AV_PIX_FMT_YUV420P, AV_PIX_FMT_NV12); > + TEST_SEMIPLANAR(AV_PIX_FMT_YUV420P10, AV_PIX_FMT_P010); > + TEST_SEMIPLANAR(AV_PIX_FMT_YUV420P12, AV_PIX_FMT_P012); > + TEST_SEMIPLANAR(AV_PIX_FMT_YUV420P16, AV_PIX_FMT_P016); > + > +#define TEST_PACKED(input, expected) \ > + test(input, expected, &pass, &fail, find_best_packed); > + > + // Same formats. > + for (i = 0; i < FF_ARRAY_ELEMS(packed_list); i++) > + TEST_PACKED(packed_list[i], packed_list[i]); > + > + // Formats containing the same data in different layouts. > + TEST_PACKED(AV_PIX_FMT_YUV444P, AV_PIX_FMT_VUYX); > + TEST_PACKED(AV_PIX_FMT_YUV444P10, AV_PIX_FMT_XV30); > + TEST_PACKED(AV_PIX_FMT_YUV444P12, AV_PIX_FMT_XV36); > + TEST_PACKED(AV_PIX_FMT_YUV422P, AV_PIX_FMT_YUYV422); > + TEST_PACKED(AV_PIX_FMT_YUV422P10, AV_PIX_FMT_Y210); > + TEST_PACKED(AV_PIX_FMT_YUV422P12, AV_PIX_FMT_Y212); > + > printf("%d tests passed, %d tests failed.\n", pass, fail); > return !!fail; > } > diff --git a/tests/ref/fate/pixfmt_best b/tests/ref/fate/pixfmt_best > index 783c5fe640..63911e7198 100644 > --- a/tests/ref/fate/pixfmt_best > +++ b/tests/ref/fate/pixfmt_best > @@ -1 +1 @@ > -75 tests passed, 0 tests failed. > +95 tests passed, 0 tests failed. Patchset LGTM and works well for me. BRs Haihao
On Thu, Sep 08, 2022 at 08:46:11PM -0700, Philip Langdale wrote: > Since introducing the various packed formats used by VAAPI (and p012), > we've noticed that there's actually a gap in how > av_find_best_pix_fmt_of_2 works. It doesn't actually assign any value > to having the same bit depth as the source format, when comparing > against formats with a higher bit depth. This usually doesn't matter, > because av_get_padded_bits_per_pixel() will account for it. > > However, as many of these formats use padding internally, we find that > av_get_padded_bits_per_pixel() actually returns the same value for the > 10 bit, 12, bit, 16 bit flavours, etc. In these tied situations, we end > up just picking the first of the two provided formats, even if the > second one should be preferred because it matches the actual bit depth. > > This bug already existed if you tried to compare yuv420p10 against p016 > and p010, for example, but it simply hadn't come up before so we never > noticed. > > But now, we actually got a situation in the VAAPI VP9 decoder where it > offers both p010 and p012 because Profile 3 could be either depth and > ends up picking p012 for 10 bit content due to the ordering of the > testing. > > In addition, in the process of testing the fix, I realised we have the > same gap when it comes to chroma subsampling - we do not favour a > format that has exactly the same subsampling vs one with less > subsampling when all else is equal. > > To fix this, I'm introducing a small score penalty if the bit depth or > subsampling doesn't exactly match the source format. This will break > the tie in favour of the format with the exact match, but not offset > any of the other scoring penalties we already have. > > I have added a set of tests around these formats which will fail > without this fix. > > Signed-off-by: Philip Langdale <philipl@overt.org> > --- > libavutil/pixdesc.c | 16 +++++- > libavutil/tests/pixfmt_best.c | 93 ++++++++++++++++++++++++++++------- > tests/ref/fate/pixfmt_best | 2 +- > 3 files changed, 89 insertions(+), 22 deletions(-) > > diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c > index d7c6ebfdc4..412e257a30 100644 > --- a/libavutil/pixdesc.c > +++ b/libavutil/pixdesc.c > @@ -3004,6 +3004,11 @@ static int get_pix_fmt_score(enum AVPixelFormat dst_pix_fmt, > if ((ret = get_pix_fmt_depth(&dst_min_depth, &dst_max_depth, dst_pix_fmt)) < 0) > return -3; > > + // Favour formats where bit depth exactly matches. If all other scoring is > + // equal, we'd rather use a lower bit depth that matches the source. > + if (src_min_depth != dst_min_depth || src_max_depth != dst_max_depth) > + score -= 64; This adds a penalty for decreasing the depth, that is under FF_LOSS_DEPTH more so if we decreas depth from 16 to 15 bit, that gives a penalty of 2 but increasing from 15 to 16 a penalty of 64. This needs an adjustment also are we intentionally treating a increase from 14 to 15 like 14 to 16 ? or do we want to prefer the closer ? a FF_LOSS_EQUAL_DEPTH / DIFFERENT_DEPTH could be added to keep this more in line with existing code > + > src_color = get_color_type(src_desc); > dst_color = get_color_type(dst_desc); > if (dst_pix_fmt == AV_PIX_FMT_PAL8) > @@ -3020,14 +3025,21 @@ static int get_pix_fmt_score(enum AVPixelFormat dst_pix_fmt, > } > > if (consider & FF_LOSS_RESOLUTION) { > + // Apply a large penalty if there's a loss of resolution, but also apply > + // a small penalty of the dst resolution is higher so that we favour > + // exactly matching formats. > if (dst_desc->log2_chroma_w > src_desc->log2_chroma_w) { > loss |= FF_LOSS_RESOLUTION; > score -= 256 << dst_desc->log2_chroma_w; > - } > + } else if (dst_desc->log2_chroma_w < src_desc->log2_chroma_w) > + score -= 32; > + > if (dst_desc->log2_chroma_h > src_desc->log2_chroma_h) { > loss |= FF_LOSS_RESOLUTION; > score -= 256 << dst_desc->log2_chroma_h; > - } > + } else if (dst_desc->log2_chroma_h < src_desc->log2_chroma_h) > + score -= 32; this would favor 410 -> 411 over 410 -> 420, i dont think that choice is correct in that specific case i do agree though with the basic idea behind this patch thx [...]
On Sat, 10 Sep 2022 17:04:32 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c > > index d7c6ebfdc4..412e257a30 100644 > > --- a/libavutil/pixdesc.c > > +++ b/libavutil/pixdesc.c > > @@ -3004,6 +3004,11 @@ static int get_pix_fmt_score(enum > > AVPixelFormat dst_pix_fmt, if ((ret = > > get_pix_fmt_depth(&dst_min_depth, &dst_max_depth, dst_pix_fmt)) < > > 0) return -3; > > + // Favour formats where bit depth exactly matches. If all > > other scoring is > > + // equal, we'd rather use a lower bit depth that matches the > > source. > > + if (src_min_depth != dst_min_depth || src_max_depth != > > dst_max_depth) > > + score -= 64; > > This adds a penalty for decreasing the depth, that is under > FF_LOSS_DEPTH > > more so if we decreas depth from 16 to 15 bit, that gives a penalty > of 2 but increasing from 15 to 16 a penalty of 64. This needs an > adjustment also are we intentionally treating a increase from 14 to > 15 like 14 to 16 ? or do we want to prefer the closer ? > > a FF_LOSS_EQUAL_DEPTH / DIFFERENT_DEPTH could be added to keep this > more in line with existing code Good point. I was thinking about this in binary terms, but you are right that the penalty should scale with the divergence, in the same way that it does for other mismatches. > > > + > > src_color = get_color_type(src_desc); > > dst_color = get_color_type(dst_desc); > > if (dst_pix_fmt == AV_PIX_FMT_PAL8) > > @@ -3020,14 +3025,21 @@ static int get_pix_fmt_score(enum > > AVPixelFormat dst_pix_fmt, } > > > > if (consider & FF_LOSS_RESOLUTION) { > > + // Apply a large penalty if there's a loss of resolution, > > but also apply > > + // a small penalty of the dst resolution is higher so that > > we favour > > + // exactly matching formats. > > if (dst_desc->log2_chroma_w > src_desc->log2_chroma_w) { > > loss |= FF_LOSS_RESOLUTION; > > score -= 256 << dst_desc->log2_chroma_w; > > - } > > + } else if (dst_desc->log2_chroma_w < > > src_desc->log2_chroma_w) > > + score -= 32; > > + > > if (dst_desc->log2_chroma_h > src_desc->log2_chroma_h) { > > loss |= FF_LOSS_RESOLUTION; > > score -= 256 << dst_desc->log2_chroma_h; > > - } > > + } else if (dst_desc->log2_chroma_h < > > src_desc->log2_chroma_h) > > + score -= 32; > > this would favor 410 -> 411 over 410 -> 420, i dont think that choice > is correct in that specific case True. I'll need to make it scale here as well. > i do agree though with the basic idea behind this patch Thanks for taking a look! --phil
diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c index d7c6ebfdc4..412e257a30 100644 --- a/libavutil/pixdesc.c +++ b/libavutil/pixdesc.c @@ -3004,6 +3004,11 @@ static int get_pix_fmt_score(enum AVPixelFormat dst_pix_fmt, if ((ret = get_pix_fmt_depth(&dst_min_depth, &dst_max_depth, dst_pix_fmt)) < 0) return -3; + // Favour formats where bit depth exactly matches. If all other scoring is + // equal, we'd rather use a lower bit depth that matches the source. + if (src_min_depth != dst_min_depth || src_max_depth != dst_max_depth) + score -= 64; + src_color = get_color_type(src_desc); dst_color = get_color_type(dst_desc); if (dst_pix_fmt == AV_PIX_FMT_PAL8) @@ -3020,14 +3025,21 @@ static int get_pix_fmt_score(enum AVPixelFormat dst_pix_fmt, } if (consider & FF_LOSS_RESOLUTION) { + // Apply a large penalty if there's a loss of resolution, but also apply + // a small penalty of the dst resolution is higher so that we favour + // exactly matching formats. if (dst_desc->log2_chroma_w > src_desc->log2_chroma_w) { loss |= FF_LOSS_RESOLUTION; score -= 256 << dst_desc->log2_chroma_w; - } + } else if (dst_desc->log2_chroma_w < src_desc->log2_chroma_w) + score -= 32; + if (dst_desc->log2_chroma_h > src_desc->log2_chroma_h) { loss |= FF_LOSS_RESOLUTION; score -= 256 << dst_desc->log2_chroma_h; - } + } else if (dst_desc->log2_chroma_h < src_desc->log2_chroma_h) + score -= 32; + // don't favor 422 over 420 if downsampling is needed, because 420 has much better support on the decoder side if (dst_desc->log2_chroma_w == 1 && src_desc->log2_chroma_w == 0 && dst_desc->log2_chroma_h == 1 && src_desc->log2_chroma_h == 0 ) { diff --git a/libavutil/tests/pixfmt_best.c b/libavutil/tests/pixfmt_best.c index 0542af494f..b5d4d04976 100644 --- a/libavutil/tests/pixfmt_best.c +++ b/libavutil/tests/pixfmt_best.c @@ -39,32 +39,59 @@ static const enum AVPixelFormat pixfmt_list[] = { AV_PIX_FMT_VAAPI, }; -static enum AVPixelFormat find_best(enum AVPixelFormat pixfmt) +static const enum AVPixelFormat semiplanar_list[] = { + AV_PIX_FMT_P016, + AV_PIX_FMT_P012, + AV_PIX_FMT_P010, + AV_PIX_FMT_NV12, +}; + +static const enum AVPixelFormat packed_list[] = { + AV_PIX_FMT_XV36, + AV_PIX_FMT_XV30, + AV_PIX_FMT_VUYX, + AV_PIX_FMT_Y212, + AV_PIX_FMT_Y210, + AV_PIX_FMT_YUYV422, +}; + +typedef enum AVPixelFormat (*find_best_t)(enum AVPixelFormat pixfmt); + +#define find_best_wrapper(name, list) \ +static enum AVPixelFormat find_best_ ## name (enum AVPixelFormat pixfmt) \ +{ \ + enum AVPixelFormat best = AV_PIX_FMT_NONE; \ + int i; \ + for (i = 0; i < FF_ARRAY_ELEMS(list); i++) \ + best = av_find_best_pix_fmt_of_2(best, list[i], \ + pixfmt, 0, NULL); \ + return best; \ +} + +find_best_wrapper(base, pixfmt_list) +find_best_wrapper(seminplanar, semiplanar_list) +find_best_wrapper(packed, packed_list) + +static void test(enum AVPixelFormat input, enum AVPixelFormat expected, + int *pass, int *fail, find_best_t find_best_fn) { - enum AVPixelFormat best = AV_PIX_FMT_NONE; - int i; - for (i = 0; i < FF_ARRAY_ELEMS(pixfmt_list); i++) - best = av_find_best_pix_fmt_of_2(best, pixfmt_list[i], - pixfmt, 0, NULL); - return best; + enum AVPixelFormat output = find_best_fn(input); + if (output != expected) { + printf("Matching %s: got %s, expected %s\n", + av_get_pix_fmt_name(input), + av_get_pix_fmt_name(output), + av_get_pix_fmt_name(expected)); + ++(*fail); + } else + ++(*pass); } int main(void) { - enum AVPixelFormat output; int i, pass = 0, fail = 0; -#define TEST(input, expected) do { \ - output = find_best(input); \ - if (output != expected) { \ - printf("Matching %s: got %s, expected %s\n", \ - av_get_pix_fmt_name(input), \ - av_get_pix_fmt_name(output), \ - av_get_pix_fmt_name(expected)); \ - ++fail; \ - } else \ - ++pass; \ - } while (0) +#define TEST(input, expected) \ + test(input, expected, &pass, &fail, find_best_base); // Same formats. for (i = 0; i < FF_ARRAY_ELEMS(pixfmt_list); i++) @@ -137,6 +164,34 @@ int main(void) // Opaque formats are least unlike each other. TEST(AV_PIX_FMT_DXVA2_VLD, AV_PIX_FMT_VDPAU); +#define TEST_SEMIPLANAR(input, expected) \ + test(input, expected, &pass, &fail, find_best_seminplanar); + + // Same formats. + for (i = 0; i < FF_ARRAY_ELEMS(semiplanar_list); i++) + TEST_SEMIPLANAR(semiplanar_list[i], semiplanar_list[i]); + + // Formats containing the same data in different layouts. + TEST_SEMIPLANAR(AV_PIX_FMT_YUV420P, AV_PIX_FMT_NV12); + TEST_SEMIPLANAR(AV_PIX_FMT_YUV420P10, AV_PIX_FMT_P010); + TEST_SEMIPLANAR(AV_PIX_FMT_YUV420P12, AV_PIX_FMT_P012); + TEST_SEMIPLANAR(AV_PIX_FMT_YUV420P16, AV_PIX_FMT_P016); + +#define TEST_PACKED(input, expected) \ + test(input, expected, &pass, &fail, find_best_packed); + + // Same formats. + for (i = 0; i < FF_ARRAY_ELEMS(packed_list); i++) + TEST_PACKED(packed_list[i], packed_list[i]); + + // Formats containing the same data in different layouts. + TEST_PACKED(AV_PIX_FMT_YUV444P, AV_PIX_FMT_VUYX); + TEST_PACKED(AV_PIX_FMT_YUV444P10, AV_PIX_FMT_XV30); + TEST_PACKED(AV_PIX_FMT_YUV444P12, AV_PIX_FMT_XV36); + TEST_PACKED(AV_PIX_FMT_YUV422P, AV_PIX_FMT_YUYV422); + TEST_PACKED(AV_PIX_FMT_YUV422P10, AV_PIX_FMT_Y210); + TEST_PACKED(AV_PIX_FMT_YUV422P12, AV_PIX_FMT_Y212); + printf("%d tests passed, %d tests failed.\n", pass, fail); return !!fail; } diff --git a/tests/ref/fate/pixfmt_best b/tests/ref/fate/pixfmt_best index 783c5fe640..63911e7198 100644 --- a/tests/ref/fate/pixfmt_best +++ b/tests/ref/fate/pixfmt_best @@ -1 +1 @@ -75 tests passed, 0 tests failed. +95 tests passed, 0 tests failed.
Since introducing the various packed formats used by VAAPI (and p012), we've noticed that there's actually a gap in how av_find_best_pix_fmt_of_2 works. It doesn't actually assign any value to having the same bit depth as the source format, when comparing against formats with a higher bit depth. This usually doesn't matter, because av_get_padded_bits_per_pixel() will account for it. However, as many of these formats use padding internally, we find that av_get_padded_bits_per_pixel() actually returns the same value for the 10 bit, 12, bit, 16 bit flavours, etc. In these tied situations, we end up just picking the first of the two provided formats, even if the second one should be preferred because it matches the actual bit depth. This bug already existed if you tried to compare yuv420p10 against p016 and p010, for example, but it simply hadn't come up before so we never noticed. But now, we actually got a situation in the VAAPI VP9 decoder where it offers both p010 and p012 because Profile 3 could be either depth and ends up picking p012 for 10 bit content due to the ordering of the testing. In addition, in the process of testing the fix, I realised we have the same gap when it comes to chroma subsampling - we do not favour a format that has exactly the same subsampling vs one with less subsampling when all else is equal. To fix this, I'm introducing a small score penalty if the bit depth or subsampling doesn't exactly match the source format. This will break the tie in favour of the format with the exact match, but not offset any of the other scoring penalties we already have. I have added a set of tests around these formats which will fail without this fix. Signed-off-by: Philip Langdale <philipl@overt.org> --- libavutil/pixdesc.c | 16 +++++- libavutil/tests/pixfmt_best.c | 93 ++++++++++++++++++++++++++++------- tests/ref/fate/pixfmt_best | 2 +- 3 files changed, 89 insertions(+), 22 deletions(-)