| 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 |
| 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 |
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).
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".
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".
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 :)
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
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(-)