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
