search

FFmpeg / FFV1

The FFV1 lossless video codec specification.
Other
154 stars 35 forks source link

Auth48 upstream take 2 #266

Closed dericed closed 3 years ago

dericed commented 3 years ago

This PR is intended to supersede https://github.com/FFmpeg/FFV1/pull/264 as a cleaner version working with a version of rfc9043.xml that is further along in the editorial process. The work isn't complete, particularly in the 'rephrasing' commit at the end, but should be soon.

dericed commented 3 years ago

Hi @michaelni @JeromeMartinez, Please review this pull request which brings the changes of AUTH48 into the repo for ongoing work on FFV1 v4.

In this review, I found a few pending issues and prepped this email back to the editors:

Thank you,

As part of updating our pull request to bring the AUTH48 updates into our work on the draft of FFV1 version 4, I noticed one more change to suggest:

1.
On lines 416 and 2071, please change the anchor from "ISO.14496-10.2014” to "ISO.14496-10.2020”. You already updated the rest of the reference to the new version.

2.
On line 2077, please update the date to <date year="2020" month="December”/>. The reference was updated elsewhere, but this value was missed.

3.
On line 272, please update the anchor name to “figureQuantizationSampleDifferenceMapping”. Perhaps this is already done.

Very minor:
4.
Line 2145 contains a tab character (only one in the document) and the attributes of the date mismatch the other expression.

IIUC there's also another issue of a potential one_state / zero_state flip that needs review. @michaelni, the commits up to the last one should be labeled well. The last one is a group of grammatical edits. If needed, I could break this down further.

dericed commented 3 years ago

nudging on this review, @JeromeMartinez @michaelni 0:) I'd like to send the message of the prior comment to AUTH48 for an update. Anything further to add?

michaelni commented 3 years ago

@JeromeMartinez, if you review and approve then i can apply it after a quick look, but iam a bit busy ATM with lots of things so i will probably need a bit time if iam the only reviewer