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

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Hendrik Leppkes
> Sent: Wednesday, June 29, 2022 9:50 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [RFC] Bug in colorspace.c
> 
> On Wed, Jun 29, 2022 at 6:34 AM Hendrik Leppkes <h.leppkes at gmail.com>
> wrote:
> >
> > On Tue, Jun 28, 2022 at 11:38 PM Soft Works <softworkz at 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