On Tue, Mar 12, 2019 at 10:21 PM Vittorio Giovara < [email protected]> wrote:
> > > On Tue, Mar 12, 2019 at 12:11 PM Aruna Matheswaran < > [email protected]> 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 < >> [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 >> > > > -- > 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
