[x265] [PATCH] Backed out changeset: fef63866bb60

[Vittorio Giovara]

On Mon, Mar 11, 2019 at 3:48 AM Pradeep Ramachandran <
pradeep at multicorewareinc.com> wrote:

> On Mon, Mar 11, 2019 at 9:17 AM Pradeep Ramachandran <
> pradeep at multicorewareinc.com> wrote:
>
>> On Sat, Mar 9, 2019 at 2:14 AM Vittorio Giovara <
>> vittorio.giovara at gmail.com> wrote:
>>
>>>
>>>
>>> On Fri, Mar 8, 2019 at 12:27 AM Pradeep Ramachandran <
>>> pradeep at multicorewareinc.com> wrote:
>>>
>>>> # HG changeset patch
>>>> # User Pradeep Ramachandran <pradeep at multicorewareinc.com>
>>>> # Date 1552022473 -19800
>>>> #      Fri Mar 08 10:51:13 2019 +0530
>>>> # Node ID 636258ebc7a90e0a35466e9b605ab335b9ce2194
>>>> # Parent  0eccd62725b6a24ae27d52189c4a624dffdd7a07
>>>> Backed out changeset: fef63866bb60
>>>>
>>>> diff -r 0eccd62725b6 -r 636258ebc7a9 source/encoder/encoder.cpp
>>>> --- a/source/encoder/encoder.cpp        Mon Mar 04 15:36:38 2019 +0530
>>>> +++ b/source/encoder/encoder.cpp        Fri Mar 08 10:51:13 2019 +0530
>>>> @@ -2459,13 +2459,10 @@
>>>>
>>>>      if (m_param->bEmitHDRSEI)
>>>>      {
>>>> -        if (m_emitCLLSEI)
>>>> -        {
>>>> -            SEIContentLightLevel cllsei;
>>>> -            cllsei.max_content_light_level = m_param->maxCLL;
>>>> -            cllsei.max_pic_average_light_level = m_param->maxFALL;
>>>> -            cllsei.writeSEImessages(bs, m_sps, NAL_UNIT_PREFIX_SEI,
>>>> list, m_param->bSingleSeiNal);
>>>> -        }
>>>> +        SEIContentLightLevel cllsei;
>>>> +        cllsei.max_content_light_level = m_param->maxCLL;
>>>> +        cllsei.max_pic_average_light_level = m_param->maxFALL;
>>>> +        cllsei.writeSEImessages(bs, m_sps, NAL_UNIT_PREFIX_SEI, list,
>>>> m_param->bSingleSeiNal);
>>>>
>>>>          if (m_param->masteringDisplayColorVolume)
>>>>          {
>>>>
>>>
>>> Why?
>>>
>>> It would be *really* nice if this kind of information was provided in
>>> the commit message without having to ask it every time.
>>> Like in the commit that is being removed: "Some devices render
>>> out-of-luminance pixels incorrectly otherwise."
>>>
>>> So, NAK until further explanation is provided.
>>>
>>
>> Apologies for not clarifying why in the commit message.
>>
>
Hello,
thanks for your reply, you should also let some time pass between sending a
patch for review and committing to master.
15 hours is definitely too little time, might as well have skipped sending
the email at all then.

Onto the main problems, I am still not convinced by your explanation and I
believe you should revert this change. See below.


> The reason for backing this commit out was because we got some error
>> reports that some packagers were treating the streams that didn't have this
>> set correctly as non-HDR10 streams and that wasn't ok. Verifiers like the
>> DoViES verifiers were complaining, and that was a problem for some users
>> breaking their streams.
>>
>
This SEI message should be used *only* for HDR10, which other non-HDR10
stream with this SEI not correctly set were not working correctly? Are you
talking about HDR10+ or DolbyVision? Either of them does not explain why it
is important to have this SEI message with empty values. As explained in
*my* commit message, if you encode a stream with mastering display *only*,
you don't want content light level to be encoded in with 0 as max average
light level and 0 as max frame light level, or tonemapping algorithms of
some devices will interpret this value incorrectly and display pixels too
dark or too bright. There is nothing wrong in encoding a stream with
mastering display only without content light level, and it is not so far
fetched to think that a renderer will blindly use whatever value it is
encoded in the SEI if the SEI is present.

On that note, verifiers *often* interpret the specifications differently,
depending on the products and affiliation, it is also possible that some
are plainly wrong. Before backing out a change with a clear explanation on
why it was applied, you should maybe reach out and make sure that the
verifier is not completely off, like in this case it seems.

And I forgot to mention that the --hdr option was added to force signalling
> of HDR params even with 0 max-cll values (please see docs at
> https://x265.readthedocs.io/en/latest/cli.html#cmdoption-hdr).
> Does excluding the --hdr option from your cli not suffice for your
> use-case? The only situation I can think of is having master-display but
> with 0 max-cll / max-fall values. Is that your use-case?
>

You can't expect your users to always use the command line interface. In
fact I use the x265 library and API: when you set mastering display
information with param_parse(), the m_param->bEmitHDRSEI is set to true,
but I do *not* want that content light level is encoded too unless they are
non-zero, which is what m_emitCLLSEI checked. With your revert, now setting
any mastering display information will automatically encode content light
level and risk producing streams that while conformant to a verifier, cause
problems in real-world video applications.

Please revert your change, write better commit messages, and allow more
time for review before disrupting video pipelines.
Best regards,
Vittorio
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20190311/7c102ebd/attachment.html>