[FFmpeg-devel] [PATCH] lavu/pixdesc: favour formats where bit depth exactly matches

[Michael Niedermayer]

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 at 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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220910/5703d84c/attachment.sig>