Re: [FFmpeg-devel] [PATCH v2 1/8] swscale: document SWS_FULL_CHR_H_* flags

[Niklas Haas <ffmpeg@haasn.xyz>]

On Thu, 04 Jul 2024 16:24:24 +0100 Andrew Sayers <ffmpeg-devel@pileofstuff.org> wrote:
> On Thu, Jul 04, 2024 at 04:30:57PM +0200, Niklas Haas wrote:
> > From: Niklas Haas <git@haasn.dev>
> > 
> > Based on my best understanding of what they do, given the source code.
> > ---
> >  libswscale/swscale.h | 28 ++++++++++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libswscale/swscale.h b/libswscale/swscale.h
> > index 9d4612aaf3..e22931cab4 100644
> > --- a/libswscale/swscale.h
> > +++ b/libswscale/swscale.h
> > @@ -82,11 +82,35 @@ const char *swscale_license(void);
> >  #define SWS_PRINT_INFO              0x1000
> >  
> >  //the following 3 flags are not completely implemented
> > -//internal chrominance subsampling info
> > +
> > +/**
> > + * Perform full chroma upsampling when converting to RGB as part of scaling.
> 
> Nitpick: "as part of scaling" seems redundant - can it be removed?

I wrote it this way because, afaict, this flag does not affect unscaled
special converters (yuv->rgba). But I can remove it if you still think
it's unnecessary.

> 
> > + *
> > + * For example, when converting 50x50 yuv420p to 100x100 rgba, setting this flag
> > + * will scale the chroma plane from 25x25 to 100x100 (4:4:4), and then convert
> > + * the 100x100 yuv444p image to rgba in the final output step.
> > + *
> > + * Without this flag, the chroma plane is instead scaled to 50x100 (4:2:2),
> > + * with a single chroma sample being re-used for both horizontally adjacent RGBA
> > + * output pixels.
> 
> Nitpick: this would be more readable as "for both of the...".
> 
> Consider the following sentence:
> 
>     Without this flag, the chroma plane is instead scaled to 50x100 (4:2:2),
>     with a single chroma sample being re-used for both horizontally and vertically
>     adjacent RGBA output pixels.
> 
> Using "both of the" would make it clear what "both" refers to before the reader
> starts doing branch-prediction in their head.

Fixed.

> 
> Otherwise, LGTM (by which I mean it's clear, not that I know whether it's
> correct).
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".