[x265] [PATCH] Backed out changeset: fef63866bb60
Aruna Matheswaran
aruna at multicorewareinc.com
Tue Mar 12 17:10:46 CET 2019
Thanks for the pointers, Vittorio. CTA-861.3-A specification states that if
both MaxCLL and MaxFALL are signaled as 0, the rendering device shall
interpret it as unknown. With this reference, x265 by default is signaling
0 for both MaxCLL and MaxFALL with the assumption that any logical
implementation of the specification would ignore them.
The problem we see now is that your renderer interprets 0 content light
levels as valid values and displays too dark or too bright pixels. Whereas
a few other renderers don't accept NULL entries for content light levels
and expect 0 content light level as a signal to disable/ignore their usage.
Will introducing *an additional param flag to enable signaling of only
mastering display metadata *fix your problem? With this, renderers which
don't accept NULL content light level entries shall use the default 0
signaling. On the other hand, renderers which treat 0 content light level
as valid entries shall disable signaling them via the additional flag.
Please share your thoughts on this suggestion.
Thanks,
Aruna
On Mon, Mar 11, 2019 at 7:40 PM Vittorio Giovara <vittorio.giovara at gmail.com>
wrote:
>
>
>
> 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
>
> _______________________________________________
> x265-devel mailing list
> x265-devel at videolan.org
> https://mailman.videolan.org/listinfo/x265-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20190312/5f296f4a/attachment.html>
More information about the x265-devel
mailing list