Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Clarification about when CodecPrivate is mandatory #446
base: master
Are you sure you want to change the base?
Clarification about when CodecPrivate is mandatory #446
Changes from all commits
1d11067
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The SHOULD should probably be a MUST and mention that the line is discarded. But the wording for this whole codec is questionable and could be fixed separately.
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.
poke
If we leave SHOULD we have to explain in which case do use it and in which case we don't.
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.
Should be
CodecPrivate
notPrivate Data
.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.
IMO present and with a size of 0 would also mean 'none'. For example in VP9 you can have one to give the 10/12 bits profiles. But there may be none. IMO writing an empty field is better than no field at all in this case. It's clear that it's the default profile. (there are tons of 10 bits VP9 files with no CodecPrivate and there's no way to tell if they are 8 or 10 bits).
This is also in line with
V_PRORES
would probably benefit from that as well.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.
Since we're trying to stay away from "empty elements" this comment is not valid anymore.
The VP9 case still stands. Although there is some definition of what to put in the CodecPrivate, in most cases the element is not there at all. It doesn't fall in neither the
no initialization
nor theinitialization to 'none'
categories. So in the end it should be up to the codec to decide when it makes sense to put it and what it means when it's not there (for VP9 that means 8 bits and some unknown profile).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.
poke
IMO this addition is not good, it's up to the codec to tell what it means if it's present or not.