[FFmpeg-devel] [RFC] Bug in colorspace.c

[FFmpeg-devel] [RFC] Bug in colorspace.c

[Soft Works]

Hi,

in colorspace.c, there are two functions with contradicting behavior
regarding reading/writing the max_luminance value from/to 
AVMasteringDisplayMetadata. The code seems to be unchanged since 
3 years:


1. ff_determine_signal_peak()

    sd = av_frame_get_side_data(in, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
    if (!peak && sd) {
        AVMasteringDisplayMetadata *metadata = (AVMasteringDisplayMetadata *)sd->data;
        if (metadata->has_luminance)
            peak = av_q2d(metadata->max_luminance) / REFERENCE_WHITE;
    }


2. ff_update_hdr_metadata()

    sd = av_frame_get_side_data(in, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
    if (sd) {
        AVMasteringDisplayMetadata *metadata = (AVMasteringDisplayMetadata *)sd->data;
        if (metadata->has_luminance)
            metadata->max_luminance = av_d2q(peak * REFERENCE_WHITE, 10000);
    }


The latter function writes the value as an AVRational with a denominator of
10000, but the former function doesn't multiply the value by 10000. 

So - both cannot be right.

In fact, there seems to be some confusion about the base/range of this value. 
I had found contradicting code somewhere in libva or Intel Media Driver
code a while ago, and I have also seen videos with both "kinds" of values.

This might be a chicken-egg issue, though: maybe those other files I've seen
were created by ffmpeg and subject to this bug. I'm not sure..

As I needed a practical rather than an academical solution, I ended up using a 
check on the value range to determine whether to multiply by 10000 or not
(function 1). I won't submit this as a patch because somebody would shout out
"hack" anyway, while I see it more as an adaption to reality :-)

Anyway, the second function for writing the value should definitely do it in
the right way - unfortunately it can't do without a sane input parameter, 
which in turn can only be ensured with said "hack".

For now, I just want to point at the problem as I don't have a clean solution
to offer, but I'd be interested in others' thoughts.


Best wishes,
softworkz










_______________________________________________
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".

Re: [FFmpeg-devel] [RFC] Bug in colorspace.c

[Hendrik Leppkes]

On Tue, Jun 28, 2022 at 11:38 PM Soft Works <softworkz@hotmail.com> wrote:
>
> Hi,
>
> in colorspace.c, there are two functions with contradicting behavior
> regarding reading/writing the max_luminance value from/to
> AVMasteringDisplayMetadata. The code seems to be unchanged since
> 3 years:
>
>
> 1. ff_determine_signal_peak()
>
>     sd = av_frame_get_side_data(in, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
>     if (!peak && sd) {
>         AVMasteringDisplayMetadata *metadata = (AVMasteringDisplayMetadata *)sd->data;
>         if (metadata->has_luminance)
>             peak = av_q2d(metadata->max_luminance) / REFERENCE_WHITE;
>     }
>
>
> 2. ff_update_hdr_metadata()
>
>     sd = av_frame_get_side_data(in, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
>     if (sd) {
>         AVMasteringDisplayMetadata *metadata = (AVMasteringDisplayMetadata *)sd->data;
>         if (metadata->has_luminance)
>             metadata->max_luminance = av_d2q(peak * REFERENCE_WHITE, 10000);
>     }
>
>
> The latter function writes the value as an AVRational with a denominator of
> 10000, but the former function doesn't multiply the value by 10000.
>

These two calls round-trip just fine. The 10000 is not a forced
denominator, divisor, or multiplier, it is the maximum denominator it
is allowed to use to convert the floating point value to a rational.
av_d2q will ensure the value is properly scaled.

- Hendrik
_______________________________________________
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".

Re: [FFmpeg-devel] [RFC] Bug in colorspace.c

[Hendrik Leppkes]

On Wed, Jun 29, 2022 at 6:34 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
> On Tue, Jun 28, 2022 at 11:38 PM Soft Works <softworkz@hotmail.com> wrote:
> >
> > Hi,
> >
> > in colorspace.c, there are two functions with contradicting behavior
> > regarding reading/writing the max_luminance value from/to
> > AVMasteringDisplayMetadata. The code seems to be unchanged since
> > 3 years:
> >
> >
> > 1. ff_determine_signal_peak()
> >
> >     sd = av_frame_get_side_data(in, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
> >     if (!peak && sd) {
> >         AVMasteringDisplayMetadata *metadata = (AVMasteringDisplayMetadata *)sd->data;
> >         if (metadata->has_luminance)
> >             peak = av_q2d(metadata->max_luminance) / REFERENCE_WHITE;
> >     }
> >
> >
> > 2. ff_update_hdr_metadata()
> >
> >     sd = av_frame_get_side_data(in, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
> >     if (sd) {
> >         AVMasteringDisplayMetadata *metadata = (AVMasteringDisplayMetadata *)sd->data;
> >         if (metadata->has_luminance)
> >             metadata->max_luminance = av_d2q(peak * REFERENCE_WHITE, 10000);
> >     }
> >
> >
> > The latter function writes the value as an AVRational with a denominator of
> > 10000, but the former function doesn't multiply the value by 10000.
> >
>
> These two calls round-trip just fine. The 10000 is not a forced
> denominator, divisor, or multiplier, it is the maximum denominator it
> is allowed to use to convert the floating point value to a rational.
> av_d2q will ensure the value is properly scaled.
>

To elaborate some more, AVRational is just a number, it has no
inherent "base" or "range". An AVRational of 1000/1 or 10000000/10000
represents the same value - 1000, the extra zeros have no meaning and
none should be attributed to it. Its just that, a rational value
represented as math intended - a fraction.
If any code that processes it needs the value in a particular scale,
for example the 10000 denominator scale HEVC SEI uses, then it needs
to make sure to convert it into that particular scale, as the incoming
AVRational makes absolutely no guarantees in that regard.

All code currently using eg. max_luminance in ffmpeg seems to do it
right. The AVRational is never handled directly, but typically read
through av_q2d (to convert it into a real number), or rescaled
manually to the base a format expects.
So whatever odd values you see somewhere else, its not from FFmpeg.

- Hendrik
_______________________________________________
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".

Re: [FFmpeg-devel] [RFC] Bug in colorspace.c

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Hendrik Leppkes
> Sent: Wednesday, June 29, 2022 9:50 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [RFC] Bug in colorspace.c
> 
> On Wed, Jun 29, 2022 at 6:34 AM Hendrik Leppkes <h.leppkes@gmail.com>
> wrote:
> >
> > On Tue, Jun 28, 2022 at 11:38 PM Soft Works <softworkz@hotmail.com>
> wrote:
> > >
> > > Hi,
> > >
> > > in colorspace.c, there are two functions with contradicting
> behavior
> > > regarding reading/writing the max_luminance value from/to
> > > AVMasteringDisplayMetadata. The code seems to be unchanged since
> > > 3 years:
> > >
> > >
> > > 1. ff_determine_signal_peak()
> > >
> > >     sd = av_frame_get_side_data(in,
> AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
> > >     if (!peak && sd) {
> > >         AVMasteringDisplayMetadata *metadata =
> (AVMasteringDisplayMetadata *)sd->data;
> > >         if (metadata->has_luminance)
> > >             peak = av_q2d(metadata->max_luminance) /
> REFERENCE_WHITE;
> > >     }
> > >
> > >
> > > 2. ff_update_hdr_metadata()
> > >
> > >     sd = av_frame_get_side_data(in,
> AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
> > >     if (sd) {
> > >         AVMasteringDisplayMetadata *metadata =
> (AVMasteringDisplayMetadata *)sd->data;
> > >         if (metadata->has_luminance)
> > >             metadata->max_luminance = av_d2q(peak *
> REFERENCE_WHITE, 10000);
> > >     }
> > >
> > >
> > > The latter function writes the value as an AVRational with a
> denominator of
> > > 10000, but the former function doesn't multiply the value by
> 10000.
> > >
> >
> > These two calls round-trip just fine. The 10000 is not a forced
> > denominator, divisor, or multiplier, it is the maximum denominator
> it
> > is allowed to use to convert the floating point value to a
> rational.
> > av_d2q will ensure the value is properly scaled.
> >
> 
> To elaborate some more, AVRational is just a number, it has no
> inherent "base" or "range". An AVRational of 1000/1 or 10000000/10000
> represents the same value - 1000, the extra zeros have no meaning and
> none should be attributed to it. Its just that, a rational value
> represented as math intended - a fraction.

Thanks for explaining fractions. I guess I wouldn't have been offered
doctorate @in.tum.de without knowing, but well, that's the downside
of using pseudonyms and retrospectively, I wish I had started here under
my real name.

I mistakenly assumed that av_d2q() would be a short form for creating
an AVRational with the first param being the numerator and the second
being the denominator. The assumption seemed plausible to me because
in SEI messages, the min and max luminance values need to be specified 
in units of 0.0001 cd/m² while MaxCLL is using units of cd/m².

In fact it seems that AVMasteringDisplayMetadata.max_luminance is
meant to be in cd/m², and I understand that the second parameter
in av_d2q() just limits the effective precision to 0.0001 of the
rational.

> If any code that processes it needs the value in a particular scale,
> for example the 10000 denominator scale HEVC SEI uses, then it needs
> to make sure to convert it into that particular scale, as the
> incoming
> AVRational makes absolutely no guarantees in that regard.

Yup, understood.

> All code currently using eg. max_luminance in ffmpeg seems to do it
> right. The AVRational is never handled directly, but typically read
> through av_q2d (to convert it into a real number), or rescaled
> manually to the base a format expects.
> So whatever odd values you see somewhere else, its not from FFmpeg.

I see now that my suspicion was unjustified, the confusion about the
base unit of those values must come from elsewhere. But there aren't 
just rare cases, I see it quite often, like here for example:

https://github.com/Intel-Media-SDK/MediaSDK/issues/2597#issuecomment-1072795311

The max luminance value of 40000000 seems to be realistic and makes
4000 cd/m². The default for SMPTE ST.2084 is 10000 cd/m².
But the value from the other file is 5000 which would make 0.5 cd/m²
and that doesn't appear to be a valid value, so it's probably 
erroneously specified in cd/m² instead of 0.0001 cd/m² in the SEI
message.

I ended up multiplying by 10000 in case the values are < 10 do you
think it makes sense?

Thank you very much,
softworkz
_______________________________________________
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".