<div dir="ltr"><div dir="ltr">On Tue, Mar 12, 2019 at 10:21 PM Vittorio Giovara <<a href="mailto:vittorio.giovara@gmail.com">vittorio.giovara@gmail.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Mar 12, 2019 at 12:11 PM Aruna Matheswaran <<a href="mailto:aruna@multicorewareinc.com" target="_blank">aruna@multicorewareinc.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="ltr"><div>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. </div></div></div></blockquote><div><br></div><div>Thanks for your response, I am aware of this and it logically makes sense.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="ltr"><div>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.<br></div></div></div></blockquote><div><br></div><div>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.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="ltr"><div>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. </div></div></div></blockquote><div><br></div><div>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.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="ltr"><div></div><div>Will introducing <b>an additional param flag to enable signaling of only mastering display metadata </b>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.</div></div></div></blockquote><div><br></div><div>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.<br></div></div></div></div></blockquote><div><br></div><div>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!</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div></div><div> </div><div>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.<br></div></div></div></div></blockquote><div><br></div><div>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!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div></div><div>Regards</div><div>Vittorio<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="ltr"><div><br></div><div>Thanks,</div><div>Aruna</div><br class="gmail-m_-3862557947954596300gmail-m_6416395798482979221m_920242765906313746gmail-Apple-interchange-newline"></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 11, 2019 at 7:40 PM Vittorio Giovara <<a href="mailto:vittorio.giovara@gmail.com" rel="noreferrer" target="_blank">vittorio.giovara@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 11, 2019 at 3:48 AM Pradeep Ramachandran <<a href="mailto:pradeep@multicorewareinc.com" rel="noreferrer" target="_blank">pradeep@multicorewareinc.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr">On Mon, Mar 11, 2019 at 9:17 AM Pradeep Ramachandran <<a href="mailto:pradeep@multicorewareinc.com" rel="noreferrer" target="_blank">pradeep@multicorewareinc.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Sat, Mar 9, 2019 at 2:14 AM Vittorio Giovara <<a href="mailto:vittorio.giovara@gmail.com" rel="noreferrer" target="_blank">vittorio.giovara@gmail.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 8, 2019 at 12:27 AM Pradeep Ramachandran <<a href="mailto:pradeep@multicorewareinc.com" rel="noreferrer" target="_blank">pradeep@multicorewareinc.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"># HG changeset patch<br>
# User Pradeep Ramachandran <<a href="mailto:pradeep@multicorewareinc.com" rel="noreferrer" target="_blank">pradeep@multicorewareinc.com</a>><br>
# Date 1552022473 -19800<br>
# Fri Mar 08 10:51:13 2019 +0530<br>
# Node ID 636258ebc7a90e0a35466e9b605ab335b9ce2194<br>
# Parent 0eccd62725b6a24ae27d52189c4a624dffdd7a07<br>
Backed out changeset: fef63866bb60<br>
<br>
diff -r 0eccd62725b6 -r 636258ebc7a9 source/encoder/encoder.cpp<br>
--- a/source/encoder/encoder.cpp Mon Mar 04 15:36:38 2019 +0530<br>
+++ b/source/encoder/encoder.cpp Fri Mar 08 10:51:13 2019 +0530<br>
@@ -2459,13 +2459,10 @@<br>
<br>
if (m_param->bEmitHDRSEI)<br>
{<br>
- if (m_emitCLLSEI)<br>
- {<br>
- SEIContentLightLevel cllsei;<br>
- cllsei.max_content_light_level = m_param->maxCLL;<br>
- cllsei.max_pic_average_light_level = m_param->maxFALL;<br>
- cllsei.writeSEImessages(bs, m_sps, NAL_UNIT_PREFIX_SEI, list, m_param->bSingleSeiNal);<br>
- }<br>
+ SEIContentLightLevel cllsei;<br>
+ cllsei.max_content_light_level = m_param->maxCLL;<br>
+ cllsei.max_pic_average_light_level = m_param->maxFALL;<br>
+ cllsei.writeSEImessages(bs, m_sps, NAL_UNIT_PREFIX_SEI, list, m_param->bSingleSeiNal);<br>
<br>
if (m_param->masteringDisplayColorVolume)<br>
{<br></blockquote><div><br></div><div>Why?<br></div><div><br></div><div>It would be *really* nice if this kind of information was provided in the commit message without having to ask it every time.</div><div>Like in the commit that is being removed: "Some devices render out-of-luminance pixels incorrectly otherwise."</div><div><br></div><div>So, NAK until further explanation is provided.<br></div></div></div></div></blockquote><div><br></div><div>Apologies for not clarifying why in the commit message.</div></div></div></blockquote></div></div></div></blockquote><div><br></div><div><div>Hello,</div><div>thanks for your reply, you should also let some time pass between sending a patch for review and committing to master.</div><div>15 hours is definitely too little time, might as well have skipped sending the email at all then.<br></div><div><br></div><div>Onto the main problems, I am still not convinced by your explanation and I believe you should revert this change. See below.<br></div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>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.</div></div></div></blockquote></div></div></div></blockquote></div><div class="gmail_quote"><br></div><div class="gmail_quote">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.</div><div class="gmail_quote"><br></div><div class="gmail_quote">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.<br></div><div class="gmail_quote"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div>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 <a href="https://x265.readthedocs.io/en/latest/cli.html#cmdoption-hdr" rel="noreferrer" target="_blank">https://x265.readthedocs.io/en/latest/cli.html#cmdoption-hdr</a>).</div><div>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?</div></div></div></div></blockquote><div><br></div><div>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.<br></div><div><br></div><div>Please revert your change, write better commit messages, and allow more time for review before disrupting video pipelines.</div><div>Best regards,</div><div>Vittorio<br></div><div><br></div></div></div>
_______________________________________________<br>
x265-devel mailing list<br>
<a href="mailto:x265-devel@videolan.org" rel="noreferrer" target="_blank">x265-devel@videolan.org</a><br>
<a href="https://mailman.videolan.org/listinfo/x265-devel" rel="noreferrer noreferrer" target="_blank">https://mailman.videolan.org/listinfo/x265-devel</a><br>
</blockquote></div>
_______________________________________________<br>
x265-devel mailing list<br>
<a href="mailto:x265-devel@videolan.org" target="_blank">x265-devel@videolan.org</a><br>
<a href="https://mailman.videolan.org/listinfo/x265-devel" rel="noreferrer" target="_blank">https://mailman.videolan.org/listinfo/x265-devel</a><br>
</blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail-m_-3862557947954596300gmail_signature">Vittorio</div></div></div>
_______________________________________________<br>
x265-devel mailing list<br>
<a href="mailto:x265-devel@videolan.org" target="_blank">x265-devel@videolan.org</a><br>
<a href="https://mailman.videolan.org/listinfo/x265-devel" rel="noreferrer" target="_blank">https://mailman.videolan.org/listinfo/x265-devel</a><br>
</blockquote></div></div>