Skip to content
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

More AD review updates #199

Closed
wants to merge 17 commits into from

Conversation

JeromeMartinez
Copy link
Contributor

This is part of the fixes requested in an IETF AD review.
A detailed answer, line per line of the review, is coming.

Copy link
Contributor

@dericed dericed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@michaelni
Copy link
Member

"More AD review updates"

Please split this in one commit per fix with a terse but larger than 0 bytes explanation for each

@michaelni
Copy link
Member

Removing english definitions of intermediates does not seem a good idea to me
random example: "the slice vertical position in pixels."
this can surely all be defined by code expressions but that could make it harder to understand

@JeromeMartinez
Copy link
Contributor Author

Removing english definitions of intermediates does not seem a good idea to me

I misunderstood the AD request. I put chapters and definitions back. I kept the change of how to indicate the definition of plane_pixel_height and plane_pixel_width (separated feedback about the weirdness of the definition, and now it is more coherent with how it is done for other definitions).

Please split this in one commit per fix

Done. please review again.

Copy link
Contributor

@dericed dericed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-reviewed after the pr was rebased and this looks good to me. There are some remaining issues from Murray's review that I can fix, so I send a pull request after this one is merged.

@dericed dericed mentioned this pull request May 3, 2020
@dericed
Copy link
Contributor

dericed commented May 4, 2020

nudging here to be responsive to the ietf review.

@michaelni
Copy link
Member

Ive merged a bunch of these. Some hesitation i had with
"... a content with ..." that sounds a bit odd has someone who knows english well approved this ?

"SHOULD reject a content with version == 2"

Why should a version 3 or 4 specification contain such a recommandition ?

-If colorspace_type is 1, then chroma_planes MUST be 1, log2_h_chroma_subsample MUST be 0, and log2_v_chroma_subsample MUST be 0.
+Decoders SHOULD reject a content with colorspace_type == 1 && (chroma_planes != 1 || log2_h_chroma_subsample != 0 || log2_v_chroma_subsample != 0).

First and foremost we need to describe what is the correct bitstream and what is the correct output for an encoder to generate. Otherwise we will end up with incorrect bitstreams.
Other specifications generally do not describe what a decoder should do with things that are not valid. I see no reason why we should stray from this path ISO/ITU are using.

Move note for future implementation to an appendix

I dunno, that does more than move

i also see "Warning: Too long line found (L770), 5 characters longer than 72 characters:" not sure where this was introduced

@JeromeMartinez
Copy link
Contributor Author

I close this PR it as it is superseded by #202.

@JeromeMartinez JeromeMartinez deleted the more-AD-review-updates branch May 26, 2020 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants