[x265] [PATCH] Backed out changeset: fef63866bb60
Vittorio Giovara
vittorio.giovara at gmail.com
Tue Mar 12 17:51:08 CET 2019
On Tue, Mar 12, 2019 at 12:11 PM Aruna Matheswaran <
aruna at multicorewareinc.com> wrote:
> 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.
>
Thanks for your response, I am aware of this and it logically makes sense.
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.
>
This part I don't understand. The possibility of avoiding sending this SEI
is just one if clause, what is the purpose of encoding an empty message? Is
it a requirement for some other specification? Does it serve a private x265
use? Nothing wrong in either, but please have it documented somewhere.
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.
>
Unfortunately though it is not *my* renderer, but it's the renderer of some
tvs and devices in the wild, over which I have no control.
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.
>
This would kind of work but I do not believe it's a proper solution. At
most, the default behavior should be the one of least expected surprise: if
message is empty just don't encode it. Then if a sensible usecase really
exists, there should be an option to force encode light level even if
empty. However it's still unclear why you would need to that in the first
place, as trusting decoders to do the right thing is not very efficient and
leads to a catch-a-mole experience.
Apologies if my previous email had a "Wrong On The Internet" tone on it, I
was really annoyed by the sequence of events, but I could have been more
constructive and explained my points more politely.
Regards
Vittorio
> 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
>>
> _______________________________________________
> x265-devel mailing list
> x265-devel at videolan.org
> https://mailman.videolan.org/listinfo/x265-devel
>
--
Vittorio
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20190312/d8d87f2b/attachment-0001.html>
More information about the x265-devel
mailing list