-
Notifications
You must be signed in to change notification settings - Fork 15
Clarify the color info in the codecs string #142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify the color info in the codecs string #142
Conversation
e7dc5d5 to
09c653f
Compare
|
The changes in the first part (#126) implement what was suggested in the issue. We realize this is a normative change with potential backwards compatibility issues. We need to check with implementations. For the codecs parameter changes (#129), we would prefer:
In both cases, we should document before and after behavior. |
09c653f to
0d05719
Compare
|
We should update the PR again to cover the case of monochrome and chromaSubsampling. They cannot be present if the following fields are omitted (and vice versa). |
|
The codecs parameter is also allowed to omit the trailing values if the colr box or the sequence header OBU contains the default values. |
|
We also need to document the before and after cases, showing possible backwards compatibility changes. |
0d05719 to
5e7c696
Compare
2eb4e0f to
3d430f4
Compare
|
The new version of the pull request has addressed all the review comments except the following:
|
|
Question, should |
|
We should split the PR into 2:
|
|
For the The summary for
|
|
Regarding the
It does not enable 2/2/2 for DV for example. Also the "it" is not clear. Simply deleting the sentence is good, or moving it into the "if |
|
I wrote the pull request #167 to clarify the |
3d430f4 to
41a67a8
Compare
|
I updated this pull request to the current main branch and edited the subject line and commit message accordingly. This pull request now addresses #129 only. |
141ac84 to
523bc57
Compare
|
Some notes on the PR:
|
523bc57 to
9a066ee
Compare
|
Maybe one more thing to consider. The HLS spec. also defines a VIDEO-RANGE definitionVIDEO-RANGEI think it would be useful if we at least add a note, mentioning that care needs to be taken as the |
|
We could add a note that says: |
|
I added the NOTE that Cyril suggested. I also move the common text about |
|
Cyril, I have some questions about your comment on Jul 10.
I assume "nclc" is a typo for "'rICC".
Could you clarify what you meant by "anything else that nclx requires going into the bitstream"? I wonder if you meant to say "anything other than nclx requires going into the bitstream". By "going into the bitstream", you meant going into the Sequence Header OBU, right?
Would you still like me to add this note? Thanks. |
This enables 2/2/2 for DV.
If color_description_present_flag is set to 0 in the Sequence Header OBU, the colorPrimaries, transferCharacteristics, matrixCoefficients parameter values SHOULD be set to the default values 01.01.01. This change maintains backward compatibility with v1.2.0, which says (after paraphrasing): if color_description_present_flag is set to 0, the colorPrimaries, transferCharacteristics, matrixCoefficients, and videoFullRangeFlag parameter values SHOULD not be set, defaulting to the values 01.01.01.0. NOTE: The color_range flag is always present in the Sequence Header OBU, so the current draft specifies that the videoFullRangeFlag parameter value SHALL be set to the color_range flag in the Sequence Header OBU.
to be consistent with the prevalent style in this section.
c23000b to
7fc9dea
Compare
|
Dimitri wrote:
Should I add the note that Dimitri suggested? The HLS spec that Dimitri mentioned is still an Internet Draft. It seems premature to mention it. |
No. This was an example. |
Yes, that's what I meant.
Yes. |
Yes, that couldn't hurt. |
No. We said that Dimitri will open a separate issue to discuss HLS. Let's not overload this PR too much. |
|
OK, I have addressed all the comments on this pull request. |
Co-authored-by: Cyril Concolato <[email protected]>
|
Unless objections in the coming days I will merge this PR. Thank you @wantehchang |
|
|
||
| The videoFullRangeFlag is represented by a single digit. | ||
|
|
||
| <assert>If the codecs parameter string ends with ".0.110.01.01.01.0" (containing all the default values below), that trailing part of the string SHOULD be omitted</assert>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could be changed from SHOULD to MAY? I think it would be an implementers choice to include it or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment. It's a SHOULD rather than a MAY because it is inefficient to include all the default values. But in the end, it's an implementer's choice. The SHOULD just gives more indication as to what needs to be done than a MAY. Unless we have a use case that needs to include the default values, I think we should keep the SHOULD.
What are you commenting on? |
…tring-note SHA: 014d8ea Reason: push, by cconcolato Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>




Fix #129.
Preview | Diff