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 <[email protected]> wrote: > > > > On Mon, Mar 11, 2019 at 3:48 AM Pradeep Ramachandran < > [email protected]> wrote: > >> On Mon, Mar 11, 2019 at 9:17 AM Pradeep Ramachandran < >> [email protected]> wrote: >> >>> On Sat, Mar 9, 2019 at 2:14 AM Vittorio Giovara < >>> [email protected]> wrote: >>> >>>> >>>> >>>> On Fri, Mar 8, 2019 at 12:27 AM Pradeep Ramachandran < >>>> [email protected]> wrote: >>>> >>>>> # HG changeset patch >>>>> # User Pradeep Ramachandran <[email protected]> >>>>> # 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 > [email protected] > https://mailman.videolan.org/listinfo/x265-devel >
_______________________________________________ x265-devel mailing list [email protected] https://mailman.videolan.org/listinfo/x265-devel
