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

Ballot roman, security part, alt #250

Merged
merged 1 commit into from
Oct 26, 2020

Conversation

dericed
Copy link
Contributor

@dericed dericed commented Oct 19, 2020

this is an alternate to #249

Copy link
Contributor

@JeromeMartinez JeromeMartinez left a comment

Choose a reason for hiding this comment

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

2 remarks + Please combine to 1 commit, as in practice it is a "move".
Maybe MediaConch implementation could also be added, the 3 conflicts between spec without the exceptions and implementation where found during its development.

@@ -276,7 +276,7 @@ top16s = t >= 32768 ? ( t - 65536 ) : t
diag16s = tl >= 32768 ? ( tl - 65536 ) : tl
```

Background: a two's complement 16-bit signed integer was used for storing Sample values in all known implementations of FFV1 bitstream. So in some circumstances, the most significant bit was wrongly interpreted (used as a sign bit instead of the 16th bit of an unsigned integer). Note that when the issue was discovered, the only configuration of all known implementations being impacted is 16-bit YCbCr with no Pixel transformation with Range Coder coder, as other potentially impacted configurations (e.g. 15/16-bit JPEG2000-RCT with Range Coder coder, or 16-bit content with Golomb Rice coder) were implemented nowhere [@!ISO.15444-1.2016]. In the meanwhile, 16-bit JPEG2000-RCT with Range Coder coder was implemented without this issue in one implementation and validated by one conformance checker. It is expected (to be confirmed) to remove this exception for the median predictor in the next version of the FFV1 bitstream.
Background: a two's complement 16-bit signed integer was used for storing Sample values in all known implementations of FFV1 bitstream (see (#ffv1-implementations)). So in some circumstances, the most significant bit was wrongly interpreted (used as a sign bit instead of the 16th bit of an unsigned integer). Note that when the issue was discovered, the only configuration of all known implementations being impacted is 16-bit YCbCr with no Pixel transformation with Range Coder coder, as other potentially impacted configurations (e.g. 15/16-bit JPEG2000-RCT with Range Coder coder, or 16-bit content with Golomb Rice coder) were implemented nowhere [@!ISO.15444-1.2016]. In the meanwhile, 16-bit JPEG2000-RCT with Range Coder coder was implemented without this issue in one implementation and validated by one conformance checker. It is expected (to be confirmed) to remove this exception for the median predictor in the next version of the FFV1 bitstream.
Copy link
Contributor

Choose a reason for hiding this comment

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

the "all known implementations of FFV1 bitstream" wording is 3x in the spec, so this change should be 3x.

<front>
<title>FFV1 Decoder in Go</title>
<author initials="D." surname="Buitenhuis" fullname="Derek Buitenhuis"/>
<date year="undated" />
Copy link
Contributor

Choose a reason for hiding this comment

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

undated

2019?

@dericed dericed force-pushed the Ballot-Roman,-security-part,-alt branch from 9bf581d to 5dc1269 Compare October 19, 2020 16:31
@dericed
Copy link
Contributor Author

dericed commented Oct 19, 2020

I added a piece about the FFV1 implementation within MediaConch to the draft appendix on notable FFV1 implementations. CC'ing @mcr on this though in regards to any particular conflicts of interest here as both myself and @JeromeMartinez managed the referenced MediaConch project.

@mcr
Copy link

mcr commented Oct 19, 2020

Pointing to reference implementations as a non-normative reference shouldn't be a problem.

@michaelni michaelni merged commit 5dc1269 into FFmpeg:master Oct 26, 2020
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.

4 participants