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

Range terminator #137

Closed
wants to merge 3 commits into from
Closed

Conversation

michaelni
Copy link
Member

No description provided.

ffv1.md Outdated
@@ -933,8 +953,8 @@ pseudo-code | type
ConfigurationRecord( NumBytes ) { |
ConfigurationRecordIsPresent = 1 |
Parameters( ) |
while( remaining_bits_in_bitstream( NumBytes ) > 32 ) |
reserved_for_future_use | u(1)
while( remaining_symbols_in_syntax ) |
Copy link
Contributor

Choose a reason for hiding this comment

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

should be a function (with parenthesis) and with a description as remaining_bits_in_bitstream() is.

ffv1.md Outdated

The range coder can be used in 3 modes.

--------------- ----------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

This ----- ----- format of indentation doesn't have an easy equivalent in RFC documents. Could this be a table or list instead?

ffv1.md Outdated
The range coder can be used in 3 modes.

--------------- ----------------------------------------------------------------
`Open mode` In which when decoding, every symbol the reader attempts to read is available. In this mode arbitrary data can have been appended without affecting the range coder output. This mode is not used in FFv1.
Copy link
Contributor

Choose a reason for hiding this comment

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

used in FFV1 (all caps letters)

ffv1.md Outdated

`Closed mode` In which the length in bytes of the bytestream is provided to the range decoder. Bytes beyond the length are read as 0 by the range decoder. This is generally 1 byte shorter than the open mode.

`Sentinel mode` In which case the exact length in bytes is not known. Thus the range decoder may read into non range coded data following range coded data. In this mode after all range coded symbols one binary symbol with state 129 is read and the value discarded. After reading this symbol the range decoder will have read one byte beyond the end of the range coded bytestream. This way the byte position of the end can be determined. Bytestreams written in sentinel mode can be read in Closed mode if the length can be determined, in this case the last (sentinel) symbol will be read non corrupted and be of value 0.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an attempted re-write for clarity (but not certain I'm fully understanding):

In Sentinel mode the exact length in bytes is not known and thus the range decoder MAY read into the data the follows the range coded bytestream by one byte. In Sentinel mode, the end of the range coded bytestream is marked by a binary symbol with state 129, which value SHALL be discarded. After reading this symbol, the range decoder will have read one byte beyond the end of the range coded bytestream. This way the byte position of the end can be determined. Bytestreams written in Sentinel mode can be read in Closed mode if the length can be determined, in this case the last (sentinel) symbol will be read non-corrupted and be of value 0.

Also I notice that the phrase , which is 128 was deleted above, but here with state 129 is added. I don't have much background in range coding but hope the inconsistency makes sense.

Also I understand that a binary symbol with state 129 marks the end of the range coding, but is this binary symbol feasible in range coded data, i.e. is it possible for a false end to be read? Or is a constraint for the range coded needed to prevent it from write a binary symbol with state 129 which is not the end of the range coded data?

Since the range decoder can read beyond the range coded data, should we add a statement for the security consideration section?

Should we say anything about the case where the file ends with a sentinel mode range coded bytestream? Could the range decoder read beyond the end of the file?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is an attempted re-write for clarity (but not certain I'm fully understanding):

In Sentinel mode the exact length in bytes is not known and thus the range decoder MAY read into the data the follows the range coded bytestream by one byte. In Sentinel mode, the end of the range coded bytestream is marked by a binary symbol with state 129, which value SHALL be discarded. After reading this symbol, the range decoder will have read one byte beyond the end of the range coded bytestream. This way the byte position of the end can be determined. Bytestreams written in Sentinel mode can be read in Closed mode if the length can be determined, in this case the last (sentinel) symbol will be read non-corrupted and be of value 0.

Will use in my next iteration

Also I understand that a binary symbol with state 129 marks the end of the range coding, but is this binary symbol feasible in range coded data, i.e. is it possible for a false end to be read? Or is a constraint for the range coded needed to prevent it from write a binary symbol with state 129 which is not the end of the range coded data?

state 129 can occur anywhere, Its just special in that it works for termination

Since the range decoder can read beyond the range coded data, should we add a statement for the security consideration section?

i guess we should

Should we say anything about the case where the file ends with a sentinel mode range coded bytestream? Could the range decoder read beyond the end of the file?

I dont understand the question

ffv1.md Outdated

Above describes the range decoding, encoding is defined as any process which produces a decodable bytestream.

There are 3 places where range coder termination is needed in FFv1.
Copy link
Contributor

Choose a reason for hiding this comment

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

FFV1

ffv1.md Outdated
First is in the ConfigurationRecord, in this case the size of the range coded bytestream is known, this is handled as Closed mode.
Second is the switch from the slice header which is range coded to golomb coded slices. This is handled as Sentinel mode.
Third is the end of range coded slices which need to terminate before the CRC at their end. This can be handled as Sentinel mode
or as Closed mode if the CRC position has been determined.
Copy link
Contributor

Choose a reason for hiding this comment

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

rewrite to make references consistent:

"First is in the `Configuration Record`, in this case the size of the range coded bytestream is known and handled as `Closed mode`.
Second is the switch from the `Slice Header` which is range coded to Golomb coded slices as `Sentinel mode`.
Third is the end of range coded Slices which need to terminate before the CRC at their end. This can be handled as `Sentinel mode` or as `Closed mode` if the CRC position has been determined."

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
This issue was introduced very long ago (ebe963b)
The range coder must know the size in this specific case to do the un-termination
Thus it is not possible to add symbols after the termination but it should be
possible to add them before. So that is the location where extensions should go

I am not 100% satisfied with the way this is specified in this change, so if someone
sees a better way to specify this, contact me, send a pull req.

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
@michaelni
Copy link
Member Author

If noone has more comments (I think i took care of all from the last round) then i intend to apply this "soon"

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.

3 participants