diff mbox series

[FFmpeg-devel,v3] avutil/pixfmt: improve definition of AVColorRange

Message ID 20200920171212.26998-1-jeebjp@gmail.com
State Accepted
Commit d8ce8e8ed37e05136c96ac61f7aee83638d0600a
Headers show
Series [FFmpeg-devel,v3] avutil/pixfmt: improve definition of AVColorRange | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Jan Ekström Sept. 20, 2020, 5:12 p.m. UTC 
As it was brought up that the current documentation leaves things
as specific to YCbCr only, ICtCp and RGB are now mentioned.
Additionally, the specifications on which these definitions of
narrow and full range are defined are mentioned.

This way, the documentation of AVColorRange should now match how
most people seem to read interpret it at this point, and thus
flagging RGB AVFrames as full range is valid not only according to
common sense, but also the enum definition.
---
 libavutil/pixfmt.h | 54 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)

Comments

Vittorio Giovara Sept. 20, 2020, 6:26 p.m. UTC | #1 
On Sun, Sep 20, 2020 at 7:12 PM Jan Ekström <jeebjp@gmail.com> wrote:

> As it was brought up that the current documentation leaves things
> as specific to YCbCr only, ICtCp and RGB are now mentioned.
> Additionally, the specifications on which these definitions of
> narrow and full range are defined are mentioned.
>
> This way, the documentation of AVColorRange should now match how
> most people seem to read interpret it at this point, and thus
> flagging RGB AVFrames as full range is valid not only according to
> common sense, but also the enum definition.
> ---
>  libavutil/pixfmt.h | 54 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
> index a46acf3c5e..46ef211add 100644
> --- a/libavutil/pixfmt.h
> +++ b/libavutil/pixfmt.h
> @@ -530,12 +530,60 @@ enum AVColorSpace {
>  };
>
>  /**
> - * MPEG vs JPEG YUV range.
> + * Visual content value range.
> + *
> + * These values are based on definitions that can be found in multiple
> + * specifications, such as ITU-T BT.709 (3.4 - Quantization of RGB,
> luminance
> + * and colour-difference signals), ITU-T BT.2020 (Table 5 - Digital
> + * Representation) as well as ITU-T BT.2100 (Table 9 - Digital 10- and
> 12-bit
> + * integer representation). At the time of writing, the BT.2100 one is
> + * recommended, as it also defines the full range representation.
> + *
> + * Common definitions:
> + *   - For RGB and luminance planes such as Y in YCbCr and I in ICtCp,
> + *     'E' is the original value in range of 0.0 to 1.0.
> + *   - For chrominance planes such as Cb,Cr and Ct,Cp, 'E' is the original
> + *     value in range of -0.5 to 0.5.
> + *   - 'n' is the output bit depth.
> + *   - For additional definitions such as rounding and clipping to valid n
> + *     bit unsigned integer range, please refer to BT.2100 (Table 9).
>   */
>  enum AVColorRange {
>      AVCOL_RANGE_UNSPECIFIED = 0,
> -    AVCOL_RANGE_MPEG        = 1, ///< the normal 219*2^(n-8) "MPEG" YUV
> ranges
> -    AVCOL_RANGE_JPEG        = 2, ///< the normal     2^n-1   "JPEG" YUV
> ranges
> +
> +    /**
> +     * Narrow or limited range content.
> +     *
> +     * - For luminance planes:
> +     *
> +     *       (219 * E + 16) * 2^(n-8)
> +     *
> +     *   F.ex. the range of 16-235 for 8 bits
> +     *
> +     * - For chrominance planes:
> +     *
> +     *       (224 * E + 128) * 2^(n-8)
> +     *
> +     *   F.ex. the range of 16-240 for 8 bits
> +     */
>

I might have a minor suggestion: instead of giving the example for 8 bit
only, why not provide the generic formula for any bitdepth?
If you think it's too convoluted or the wrong place for this information,
I'd still recommend adding an example for 10 bit too, since a lot of new
content is produced in 10 bit, and could be a useful reference.

Overall this is a great improvement to the existing documentation, thanks
for writing it down
Jan Ekström Sept. 20, 2020, 6:48 p.m. UTC | #2 
On Sun, Sep 20, 2020 at 9:34 PM Vittorio Giovara
<vittorio.giovara@gmail.com> wrote:
>
> On Sun, Sep 20, 2020 at 7:12 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> > As it was brought up that the current documentation leaves things
> > as specific to YCbCr only, ICtCp and RGB are now mentioned.
> > Additionally, the specifications on which these definitions of
> > narrow and full range are defined are mentioned.
> >
> > This way, the documentation of AVColorRange should now match how
> > most people seem to read interpret it at this point, and thus
> > flagging RGB AVFrames as full range is valid not only according to
> > common sense, but also the enum definition.
> > ---
> >  libavutil/pixfmt.h | 54 +++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 51 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
> > index a46acf3c5e..46ef211add 100644
> > --- a/libavutil/pixfmt.h
> > +++ b/libavutil/pixfmt.h
> > @@ -530,12 +530,60 @@ enum AVColorSpace {
> >  };
> >
> >  /**
> > - * MPEG vs JPEG YUV range.
> > + * Visual content value range.
> > + *
> > + * These values are based on definitions that can be found in multiple
> > + * specifications, such as ITU-T BT.709 (3.4 - Quantization of RGB,
> > luminance
> > + * and colour-difference signals), ITU-T BT.2020 (Table 5 - Digital
> > + * Representation) as well as ITU-T BT.2100 (Table 9 - Digital 10- and
> > 12-bit
> > + * integer representation). At the time of writing, the BT.2100 one is
> > + * recommended, as it also defines the full range representation.
> > + *
> > + * Common definitions:
> > + *   - For RGB and luminance planes such as Y in YCbCr and I in ICtCp,
> > + *     'E' is the original value in range of 0.0 to 1.0.
> > + *   - For chrominance planes such as Cb,Cr and Ct,Cp, 'E' is the original
> > + *     value in range of -0.5 to 0.5.
> > + *   - 'n' is the output bit depth.
> > + *   - For additional definitions such as rounding and clipping to valid n
> > + *     bit unsigned integer range, please refer to BT.2100 (Table 9).
> >   */
> >  enum AVColorRange {
> >      AVCOL_RANGE_UNSPECIFIED = 0,
> > -    AVCOL_RANGE_MPEG        = 1, ///< the normal 219*2^(n-8) "MPEG" YUV
> > ranges
> > -    AVCOL_RANGE_JPEG        = 2, ///< the normal     2^n-1   "JPEG" YUV
> > ranges
> > +
> > +    /**
> > +     * Narrow or limited range content.
> > +     *
> > +     * - For luminance planes:
> > +     *
> > +     *       (219 * E + 16) * 2^(n-8)
> > +     *
> > +     *   F.ex. the range of 16-235 for 8 bits
> > +     *
> > +     * - For chrominance planes:
> > +     *
> > +     *       (224 * E + 128) * 2^(n-8)
> > +     *
> > +     *   F.ex. the range of 16-240 for 8 bits
> > +     */
> >
>
> I might have a minor suggestion: instead of giving the example for 8 bit
> only, why not provide the generic formula for any bitdepth?

I think I already have the formulas there ;) .

If you generate the doxygen, it even puts the formula into a nice code block.

> If you think it's too convoluted or the wrong place for this information,
> I'd still recommend adding an example for 10 bit too, since a lot of new
> content is produced in 10 bit, and could be a useful reference.
>

I think personally a single example is enough since the formulas and
definition of E and n are already there. But if anyone else feels
heavily for adding the 10 bit example values, I can add them.

Jan
Vittorio Giovara Sept. 23, 2020, 3:42 p.m. UTC | #3 
On Sun, Sep 20, 2020 at 8:48 PM Jan Ekström <jeebjp@gmail.com> wrote:

> On Sun, Sep 20, 2020 at 9:34 PM Vittorio Giovara
> <vittorio.giovara@gmail.com> wrote:
> >
> > On Sun, Sep 20, 2020 at 7:12 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >
> > > As it was brought up that the current documentation leaves things
> > > as specific to YCbCr only, ICtCp and RGB are now mentioned.
> > > Additionally, the specifications on which these definitions of
> > > narrow and full range are defined are mentioned.
> > >
> > > This way, the documentation of AVColorRange should now match how
> > > most people seem to read interpret it at this point, and thus
> > > flagging RGB AVFrames as full range is valid not only according to
> > > common sense, but also the enum definition.
> > > ---
> > >  libavutil/pixfmt.h | 54 +++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 51 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
> > > index a46acf3c5e..46ef211add 100644
> > > --- a/libavutil/pixfmt.h
> > > +++ b/libavutil/pixfmt.h
> > > @@ -530,12 +530,60 @@ enum AVColorSpace {
> > >  };
> > >
> > >  /**
> > > - * MPEG vs JPEG YUV range.
> > > + * Visual content value range.
> > > + *
> > > + * These values are based on definitions that can be found in multiple
> > > + * specifications, such as ITU-T BT.709 (3.4 - Quantization of RGB,
> > > luminance
> > > + * and colour-difference signals), ITU-T BT.2020 (Table 5 - Digital
> > > + * Representation) as well as ITU-T BT.2100 (Table 9 - Digital 10- and
> > > 12-bit
> > > + * integer representation). At the time of writing, the BT.2100 one is
> > > + * recommended, as it also defines the full range representation.
> > > + *
> > > + * Common definitions:
> > > + *   - For RGB and luminance planes such as Y in YCbCr and I in ICtCp,
> > > + *     'E' is the original value in range of 0.0 to 1.0.
> > > + *   - For chrominance planes such as Cb,Cr and Ct,Cp, 'E' is the
> original
> > > + *     value in range of -0.5 to 0.5.
> > > + *   - 'n' is the output bit depth.
> > > + *   - For additional definitions such as rounding and clipping to
> valid n
> > > + *     bit unsigned integer range, please refer to BT.2100 (Table 9).
> > >   */
> > >  enum AVColorRange {
> > >      AVCOL_RANGE_UNSPECIFIED = 0,
> > > -    AVCOL_RANGE_MPEG        = 1, ///< the normal 219*2^(n-8) "MPEG"
> YUV
> > > ranges
> > > -    AVCOL_RANGE_JPEG        = 2, ///< the normal     2^n-1   "JPEG"
> YUV
> > > ranges
> > > +
> > > +    /**
> > > +     * Narrow or limited range content.
> > > +     *
> > > +     * - For luminance planes:
> > > +     *
> > > +     *       (219 * E + 16) * 2^(n-8)
> > > +     *
> > > +     *   F.ex. the range of 16-235 for 8 bits
> > > +     *
> > > +     * - For chrominance planes:
> > > +     *
> > > +     *       (224 * E + 128) * 2^(n-8)
> > > +     *
> > > +     *   F.ex. the range of 16-240 for 8 bits
> > > +     */
> > >
> >
> > I might have a minor suggestion: instead of giving the example for 8 bit
> > only, why not provide the generic formula for any bitdepth?
>
> I think I already have the formulas there ;) .
>
> If you generate the doxygen, it even puts the formula into a nice code
> block.
>
> > If you think it's too convoluted or the wrong place for this information,
> > I'd still recommend adding an example for 10 bit too, since a lot of new
> > content is produced in 10 bit, and could be a useful reference.
> >
>
> I think personally a single example is enough since the formulas and
> definition of E and n are already there. But if anyone else feels
> heavily for adding the 10 bit example values, I can add them.
>

No further comments over here, push any time!
Jan Ekström Sept. 23, 2020, 4:17 p.m. UTC | #4 
On Wed, Sep 23, 2020 at 6:49 PM Vittorio Giovara
<vittorio.giovara@gmail.com> wrote:
>
> On Sun, Sep 20, 2020 at 8:48 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> > On Sun, Sep 20, 2020 at 9:34 PM Vittorio Giovara
> > <vittorio.giovara@gmail.com> wrote:
> > >
> > > On Sun, Sep 20, 2020 at 7:12 PM Jan Ekström <jeebjp@gmail.com> wrote:
> > >
> > > > As it was brought up that the current documentation leaves things
> > > > as specific to YCbCr only, ICtCp and RGB are now mentioned.
> > > > Additionally, the specifications on which these definitions of
> > > > narrow and full range are defined are mentioned.
> > > >
> > > > This way, the documentation of AVColorRange should now match how
> > > > most people seem to read interpret it at this point, and thus
> > > > flagging RGB AVFrames as full range is valid not only according to
> > > > common sense, but also the enum definition.
> > > > ---
> > > >  libavutil/pixfmt.h | 54 +++++++++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 51 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
> > > > index a46acf3c5e..46ef211add 100644
> > > > --- a/libavutil/pixfmt.h
> > > > +++ b/libavutil/pixfmt.h
> > > > @@ -530,12 +530,60 @@ enum AVColorSpace {
> > > >  };
> > > >
> > > >  /**
> > > > - * MPEG vs JPEG YUV range.
> > > > + * Visual content value range.
> > > > + *
> > > > + * These values are based on definitions that can be found in multiple
> > > > + * specifications, such as ITU-T BT.709 (3.4 - Quantization of RGB,
> > > > luminance
> > > > + * and colour-difference signals), ITU-T BT.2020 (Table 5 - Digital
> > > > + * Representation) as well as ITU-T BT.2100 (Table 9 - Digital 10- and
> > > > 12-bit
> > > > + * integer representation). At the time of writing, the BT.2100 one is
> > > > + * recommended, as it also defines the full range representation.
> > > > + *
> > > > + * Common definitions:
> > > > + *   - For RGB and luminance planes such as Y in YCbCr and I in ICtCp,
> > > > + *     'E' is the original value in range of 0.0 to 1.0.
> > > > + *   - For chrominance planes such as Cb,Cr and Ct,Cp, 'E' is the
> > original
> > > > + *     value in range of -0.5 to 0.5.
> > > > + *   - 'n' is the output bit depth.
> > > > + *   - For additional definitions such as rounding and clipping to
> > valid n
> > > > + *     bit unsigned integer range, please refer to BT.2100 (Table 9).
> > > >   */
> > > >  enum AVColorRange {
> > > >      AVCOL_RANGE_UNSPECIFIED = 0,
> > > > -    AVCOL_RANGE_MPEG        = 1, ///< the normal 219*2^(n-8) "MPEG"
> > YUV
> > > > ranges
> > > > -    AVCOL_RANGE_JPEG        = 2, ///< the normal     2^n-1   "JPEG"
> > YUV
> > > > ranges
> > > > +
> > > > +    /**
> > > > +     * Narrow or limited range content.
> > > > +     *
> > > > +     * - For luminance planes:
> > > > +     *
> > > > +     *       (219 * E + 16) * 2^(n-8)
> > > > +     *
> > > > +     *   F.ex. the range of 16-235 for 8 bits
> > > > +     *
> > > > +     * - For chrominance planes:
> > > > +     *
> > > > +     *       (224 * E + 128) * 2^(n-8)
> > > > +     *
> > > > +     *   F.ex. the range of 16-240 for 8 bits
> > > > +     */
> > > >
> > >
> > > I might have a minor suggestion: instead of giving the example for 8 bit
> > > only, why not provide the generic formula for any bitdepth?
> >
> > I think I already have the formulas there ;) .
> >
> > If you generate the doxygen, it even puts the formula into a nice code
> > block.
> >
> > > If you think it's too convoluted or the wrong place for this information,
> > > I'd still recommend adding an example for 10 bit too, since a lot of new
> > > content is produced in 10 bit, and could be a useful reference.
> > >
> >
> > I think personally a single example is enough since the formulas and
> > definition of E and n are already there. But if anyone else feels
> > heavily for adding the 10 bit example values, I can add them.
> >
>
> No further comments over here, push any time!

Thanks, applied as d8ce8e8ed37e05136c96ac61f7aee83638d0600a .

Jan
diff mbox series

Patch

diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
index a46acf3c5e..46ef211add 100644
--- a/libavutil/pixfmt.h
+++ b/libavutil/pixfmt.h
@@ -530,12 +530,60 @@  enum AVColorSpace {
 };
 
 /**
- * MPEG vs JPEG YUV range.
+ * Visual content value range.
+ *
+ * These values are based on definitions that can be found in multiple
+ * specifications, such as ITU-T BT.709 (3.4 - Quantization of RGB, luminance
+ * and colour-difference signals), ITU-T BT.2020 (Table 5 - Digital
+ * Representation) as well as ITU-T BT.2100 (Table 9 - Digital 10- and 12-bit
+ * integer representation). At the time of writing, the BT.2100 one is
+ * recommended, as it also defines the full range representation.
+ *
+ * Common definitions:
+ *   - For RGB and luminance planes such as Y in YCbCr and I in ICtCp,
+ *     'E' is the original value in range of 0.0 to 1.0.
+ *   - For chrominance planes such as Cb,Cr and Ct,Cp, 'E' is the original
+ *     value in range of -0.5 to 0.5.
+ *   - 'n' is the output bit depth.
+ *   - For additional definitions such as rounding and clipping to valid n
+ *     bit unsigned integer range, please refer to BT.2100 (Table 9).
  */
 enum AVColorRange {
     AVCOL_RANGE_UNSPECIFIED = 0,
-    AVCOL_RANGE_MPEG        = 1, ///< the normal 219*2^(n-8) "MPEG" YUV ranges
-    AVCOL_RANGE_JPEG        = 2, ///< the normal     2^n-1   "JPEG" YUV ranges
+
+    /**
+     * Narrow or limited range content.
+     *
+     * - For luminance planes:
+     *
+     *       (219 * E + 16) * 2^(n-8)
+     *
+     *   F.ex. the range of 16-235 for 8 bits
+     *
+     * - For chrominance planes:
+     *
+     *       (224 * E + 128) * 2^(n-8)
+     *
+     *   F.ex. the range of 16-240 for 8 bits
+     */
+    AVCOL_RANGE_MPEG        = 1,
+
+    /**
+     * Full range content.
+     *
+     * - For RGB and luminance planes:
+     *
+     *       (2^n - 1) * E
+     *
+     *   F.ex. the range of 0-255 for 8 bits
+     *
+     * - For chrominance planes:
+     *
+     *       (2^n - 1) * E + 2^(n - 1)
+     *
+     *   F.ex. the range of 1-255 for 8 bits
+     */
+    AVCOL_RANGE_JPEG        = 2,
     AVCOL_RANGE_NB               ///< Not part of ABI
 };