diff mbox series

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

Message ID 20240704143104.1821386-1-ffmpeg@haasn.xyz
State New
Headers show
Series [FFmpeg-devel,v2,1/8] swscale: document SWS_FULL_CHR_H_* flags | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 fail Make fate failed
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Niklas Haas July 4, 2024, 2:30 p.m. UTC
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(-)

Comments

Andrew Sayers July 4, 2024, 3:24 p.m. UTC | #1
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?

> + *
> + * 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.

Otherwise, LGTM (by which I mean it's clear, not that I know whether it's
correct).
Niklas Haas July 4, 2024, 5:56 p.m. UTC | #2
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".
Niklas Haas July 4, 2024, 5:59 p.m. UTC | #3
On Thu, 04 Jul 2024 19:56:56 +0200 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> 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.

How about: "Perform full chroma upsampling when upscaling to RGB"?

> 
> > 
> > > + *
> > > + * 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".
Andrew Sayers July 4, 2024, 6:14 p.m. UTC | #4
On Thu, Jul 04, 2024 at 07:59:10PM +0200, Niklas Haas wrote:
> On Thu, 04 Jul 2024 19:56:56 +0200 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> > 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.
> 
> How about: "Perform full chroma upsampling when upscaling to RGB"?

Ah, I hadn't understood that distinction at all.  I'd recommend...

1. keep the original if this applies to both up- and down-scaling
2. use the second if it's just for upscaling
3. either way, add a line like this at the end of the section:

    Note: this flag is ignored by unscaled special converters.

I realise this patch is just documenting current behaviour, and I'm not saying
that behaviour is correct or incorrect, but it seems important and certainly
wasn't intuitive to me.  So it's worth mentioning a bit louder :)
Niklas Haas Aug. 15, 2024, 6:02 p.m. UTC | #5
On Thu, 04 Jul 2024 16:30:57 +0200 Niklas Haas <ffmpeg@haasn.xyz> 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.
> + *
> + * 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.
> + */
>  #define SWS_FULL_CHR_H_INT    0x2000
> -//input subsampling info
> +
> +/**
> + * Perform full chroma interpolation when downscaling RGB sources.
> + *
> + * For example, when converting a 100x100 rgba source to 50x50 yuv444p, setting
> + * this flag will generate a 100x100 (4:4:4) chroma plane, which is then
> + * downscaled to the required 50x50.
> + *
> + * Without this flag, the chroma plane is instead generated at 50x100 (dropping
> + * every other pixel), before then being downscaled to the required 50x50
> + * resolution.
> + */
>  #define SWS_FULL_CHR_H_INP    0x4000
> +
>  #define SWS_DIRECT_BGR        0x8000
> +
>  #define SWS_ACCURATE_RND      0x40000
>  #define SWS_BITEXACT          0x80000
>  #define SWS_ERROR_DIFFUSION  0x800000
> -- 
> 2.45.2
> 

Forgot about this one. Merged as 68957f58..1b9c89c64.
diff mbox series

Patch

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.
+ *
+ * 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.
+ */
 #define SWS_FULL_CHR_H_INT    0x2000
-//input subsampling info
+
+/**
+ * Perform full chroma interpolation when downscaling RGB sources.
+ *
+ * For example, when converting a 100x100 rgba source to 50x50 yuv444p, setting
+ * this flag will generate a 100x100 (4:4:4) chroma plane, which is then
+ * downscaled to the required 50x50.
+ *
+ * Without this flag, the chroma plane is instead generated at 50x100 (dropping
+ * every other pixel), before then being downscaled to the required 50x50
+ * resolution.
+ */
 #define SWS_FULL_CHR_H_INP    0x4000
+
 #define SWS_DIRECT_BGR        0x8000
+
 #define SWS_ACCURATE_RND      0x40000
 #define SWS_BITEXACT          0x80000
 #define SWS_ERROR_DIFFUSION  0x800000