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

[Philip Langdale]

On Sat, 10 Sep 2022 17:04:32 +0200
Michael Niedermayer <michael at 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