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

Removing v4 (draft) related stuff. #93

Closed
wants to merge 2 commits into from

Conversation

JeromeMartinez
Copy link
Contributor

Should be put back in the a v4 branch, with more definitions (e.g. "PCM").

Issue reported by @dwbuiten

@dericed
Copy link
Contributor

dericed commented Jan 4, 2018

We should ensure this data doesn't get lost. I think better to wait a bit before forking this doc to start on a v4-specific specification, but perhaps for the meanwhile these removals should be accompanied by moving that info to a v4_notes.md document or into the issue tracker.

@retokromer retokromer mentioned this pull request Jan 4, 2018
@JeromeMartinez
Copy link
Contributor Author

v4_notes.md added, may need a rebase after #95.

v4_notes.md Outdated
@@ -0,0 +1,4 @@
# Notes for Version 4

`slice_coding_mode` was defined in a former version of the spec, it could be peek for a former version (<201-01-04)
Copy link
Contributor

Choose a reason for hiding this comment

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

2018?
Also I suggest to link to the commit with the removal in the note, i.e. 046648b.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -803,10 +803,6 @@ SliceHeader( ) { |
picture_structure | ur
sar_num | ur
sar_den | ur
if (version >= 4) { |
reset_contexts | br
Copy link
Contributor

Choose a reason for hiding this comment

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

If reset_contexts is removed here, there remain a few stranded references to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@JeromeMartinez
Copy link
Contributor Author

PR updated with reset_contexts removed (and commit hash in v4_notes.md updated).
Note that nothing says that reset_contexts and slice_coding_mode would be in v4 (they are only a suggestion in FFmpeg for the moment), so v4_notes.md is only a reminder in case we need the old content.

@dericed
Copy link
Contributor

dericed commented Jan 4, 2018

This patch also leaves a stray reference to reset_contexts in the restrictions section.

@JeromeMartinez
Copy link
Contributor Author

This patch also leaves a stray reference to reset_contexts in the restrictions section.

Oops, fixed.

Should be put back in the a v4 branch, with more definitions (e.g. "PCM")
@JeromeMartinez
Copy link
Contributor Author

Rebased.

@@ -0,0 +1,4 @@
# Notes for Version 4

`reset_contexts` and `slice_coding_mode` were defined in a former version of the spec, it could be peek for a former version (<2018-01-04), commit d712fd1.
Copy link
Contributor

Choose a reason for hiding this comment

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

You could give the link to the commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

chicken and egg: the reference Git repo is https://git.videolan.org/?p=ffmpeg.git and it does not have the commit yet.
Same for https://github.com/FFmpeg/FFV1 (a mirror) master branch, the only link available is in the PR 0fe8b02 and I am reluctant to link to a GitHub PR commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks, understood! I was thinking that a live link is more useful…

@michaelni
Copy link
Member

Isnt there some syntax with which these could be removed at "make" time ? so it all would stay in one place. That would ensure its not lost

@dericed
Copy link
Contributor

dericed commented Jan 9, 2018

@michaelni that's may be a better idea for maintenance. We could reuse the process we have for math (PDF: and RFC: prefixes) and create V013: and V4 prefixes. Any +1's?

@retokromer
Copy link
Contributor

Question: Is it useful to have V1 and V3 separated, as there are actually in the wild files from both versions?

@dericed
Copy link
Contributor

dericed commented Feb 4, 2018

@retokromer imho a discussion on having V1 and V3 separated would have been better at the time the cellar charter was reviewed and approved. I suggest to follow https://datatracker.ietf.org/doc/charter-ietf-cellar/ and not separate V1 and V3.

@dericed
Copy link
Contributor

dericed commented Feb 4, 2018

@JeromeMartinez are you in favoring of closing this in consideration of #102?

@JeromeMartinez
Copy link
Contributor Author

superseded by #102

@retokromer
Copy link
Contributor

imho a discussion on having V1 and V3 separated would have been better at the time the cellar charter was reviewed and approved

My proposal was meant only for practical reasons. I don’t need it myself anymore, as I already do have fast encoders for both v1 and v3, but I would have been grateful for having it when I programmed that stuff. And, to avoid any misunderstanding, I am very fine with not separating them.

@JeromeMartinez JeromeMartinez deleted the v3only branch June 23, 2020 19:12
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