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

Block/SimpleBlock size equations are wrong #213

Closed
mbunkus opened this issue Jan 1, 2018 · 13 comments
Closed

Block/SimpleBlock size equations are wrong #213

mbunkus opened this issue Jan 1, 2018 · 13 comments

Comments

@mbunkus
Copy link
Contributor

mbunkus commented Jan 1, 2018

The specs say the following in the section "Block Structure" and "SimpleBlock structure":

Size = 1 + (1-8) + 4 + (4 + (4)) octets. So from 6 to 21 octets.

This is, at beast, highly misleading and unclear, and at worst, just wrong.

Better numbers would be:

  • 1 byte: element ID
  • 1–8 bytes: element size (variable-sized integer)
  • 1–8 bytes: track number (variable-sized integer)
  • 2 bytes: block's timestamp relative to the cluster's timestamp (signed 16-bit integer)
  • 1 byte: flags
  • 1 byte: number of laced frames; only present if lacing is signaled in the flags
  • n bytes: lacing information; only present if lacing is signaled in the flags; total size depends on lacing method and number/size of laced frames; no upper limit (e.g. the Xiph method is rather wasteful for big frame sizes)
  • n bytes: content of laced frames

My guess is that the 1 in the equation refers to the element ID, the first 4 is supposed to be the combination of track number, relative timestamp & flags (however, that is wrong if the track number is >= 128), and I have no idea what the (4 + (4)) is supposed to convey.

It's true that the minimum number of bytes is 6, but it's completely false that the maximum number of octets is 21. Due to how lacing works, especially the incredibly bad Xiph lacing, the number of bytes doesn't really have an upper bound.

The same applies to the SimpleBlock structure & equation.

@hubblec4
Copy link
Contributor

hubblec4 commented Jan 1, 2018

To mention is that the value of the timestamp Bytes are milliseconds.

And strictly speaking, it is not a timestamp. It is time shift and with the timestamp of the Cluster and this time shift you can create a timestamp.

@mkver
Copy link
Contributor

mkver commented Jan 2, 2018

You are wrong. It needn't be milliseconds. The scale of (most) time related elements in a Matroska file is determined by the TimecodeScale element (it is TimecodeScale ns, so that the default value of 1000000 means millisecond precision and a timestamp number of n represents n*TimecodeScale ns). And both the cluster timestamp and the (relative) timestamp of (simple)blocks are ordinary timestamps in this sense.
The wordings "relative timestamp" and "timestamp relative to" are fine by me.

@retokromer
Copy link
Contributor

and I have no idea what the (4 + (4)) is supposed to convey

It is indeed a weird and confusing notation! As well as the subtraction (1-8) used for the integer interval 1 .. 8 is.

@mbunkus
Copy link
Contributor Author

mbunkus commented Jan 2, 2018

The notation lower-higher is rather common for ranges, at least here in Germany, whereas lower..higher isn't. If in doubt, we should probably chose proper mathematical notation anyway, e.g. [1, 8], because that's clearly defined.

@retokromer
Copy link
Contributor

I fully agree with using common notations, like [1, 8] which usually means a closed interval of real numbers.

@JeromeMartinez
Copy link
Contributor

The notation lower-higher is rather common for ranges, at least here in Germany, whereas lower..higher isn't

lower..higher is more common in international standard docs (e.g. MPEG or ITU), e.g. from H.265 spec:

5.7 Range notation
The following notation is used to specify a range of values:
x = y..z x takes on integer values starting from y to z, inclusive, with x, y, and z being integer numbers and z being greater than y.

@retokromer
Copy link
Contributor

I just notice that we already use the lo..hi notation for Vorbis and Theora in our specs.

@mbunkus
Copy link
Contributor Author

mbunkus commented Jan 2, 2018

Then we should stick to what we're already using.

As a matter of fact, I think we should just replace that whole formula with the enumeration I've given in the original submission, maybe supplemented by a sentence such as "The minimum number of bytes is therefore 6 in the case of a track number < 128 and no lacing."

@retokromer
Copy link
Contributor

+1 for the enumeration (it’s much clearer!)

@mbunkus
Copy link
Contributor Author

mbunkus commented Jan 2, 2018

I'll prepare a PR tonight.

@JeromeMartinez
Copy link
Contributor

JeromeMartinez commented Jan 2, 2018

I just notice that we already use the lo..hi notation for Vorbis and Theora in our specs.

Not obvious, but looks like IETF uses same, in Opus specs found 1 iteration:

it is applied independently for each set of sample S_k = {stride*n + k}, n=0..N/stride-1

Edit: argh, too late, looks like everyone is fine with double dot in that case.

@hubblec4
Copy link
Contributor

hubblec4 commented Jan 2, 2018

There is one thing which is not correct.

To describe the Bytes for the binary value of the (Simple)Block the minimum is 4 not 6.
The first entry: Element ID
and the second entry: Element Size Byte(s) are not a part of the binary value.

No other Matroska Element data will be described with the start of his own ElementID and his size.

As an example: ChapProcessData -> binary
There is just an octet preceeding these data to specify the number of commands in the element. As follows: [# of commands(1)][command 1 (8)][command 2 (8)][command 3 (8)].
1+8+(8)*
We don't set the pre-bytes for the ElementID and the size.

@mkver
A normal Matroska timestamp is scaled by the TimestampScale factor, but the "timestamp" in the Block is not scaled, or I'm wrong?

@mbunkus
Copy link
Contributor Author

mbunkus commented Jan 2, 2018

A normal Matroska timestamp is scaled by the TimestampScale factor, but the "timestamp" in the Block is not scaled, or I'm wrong?

This is wrong. The calculation is:

actual timestamp = (cluster timestamp + block's relative timestamp) * timestamp scale factor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants