[x265] [PATCH] Backed out changeset: fef63866bb60
Pradeep Ramachandran
pradeep at multicorewareinc.com
Wed Mar 13 15:41:20 CET 2019
On Tue, Mar 12, 2019 at 10:21 PM Vittorio Giovara <
vittorio.giovara at gmail.com> wrote:
>
>
> 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.
>
We have other users who've come back to us with the report that that unless
maxCLL and maxFALL are signalled as (0,0), their decoder/renderer is
decoding this as an invalid HDR10 stream. (My email earlier about non-HDR10
streams was incorrect; please ignore that.) Your use case is that your
decoder interprets (0,0) as a valid value and renders the pixels
incorrectly! As this SEI message is pass-through for the encoder, we just
went back to the standard and did what we thought was the right
interpretation of the standard, and that was to signal *all* HDR10 params
when *any* HDR10 param was non-zero. And we had another request from a user
asking for having the ability to always signal HDR10 SEIs even when they
were zero and that is why we added the --hdr option. (In hind-sight, we
should've called this --hdr10, but we will live with it for now.) Now, your
use-case is that you want a sub-set of the HDR10 SEIs to be signaled and
not the others. Maybe adding separate flags for force-signalling them
separately is the best option here, but so many flags isn't a good thing!
> 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.
>
No need for an apology here. Where is the thrill in open-source development
if there isn't a little cross-fire once in a while!
> 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
> _______________________________________________
> 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/20190313/91d91833/attachment.html>
More information about the x265-devel
mailing list