<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">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" 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" 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" 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" 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" 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>