Subject: [Cellar] Second AD review of draft-ietf-cellar-ebml-10

From: "Alexey Melnikov" <[email-masked]>
Date: Wed, 03 Jul 2019 14:44:07 +0100

Hi all,
I took over from Ben as the Area Director responsible for this WG.  My apologies that my review took so long.

Firstly, I am glad that you've implemented changes requested by Ben, as some of them were on my list of issues as well. I have a few comments of my own. Please respond to my review, so that we can collectively figure out what still needs fixing and which parts of the document I just misunderstood.

7.  EBML Element Types

   EBML Elements are defined by an EBML Schema which MUST declare one of

I think it would be helpful to readers to include a forward pointer to the section talking about EBML Schema here.

   the following EBML Element Types for each EBML Element.  An EBML
   Element Type defines a concept of storing data within an EBML Element
   that describes such characteristics as length, endianness, and
   definition.

11.1.  EBML Schema

   An EBML Schema is a well-formed XML Document that defines the

I think "well-formed XML" needs a normative reference here.

   properties, arrangement, and usage of EBML Elements that compose a
   specific EBML Document Type.

11.1.2.  <EBMLSchema> Element

   As an XML Document, the EBML Schema MUST use "<EBMLSchema>" as the
   top level element.  The "<EBMLSchema>" element MAY contain
   "<element>" sub-elements.

RFC 2119 MAY describes an implementation or deployment choice. So the above MAY doesn't seem to be appropriate. I suggest you change it to "can", as this is just a statement of fact.

11.1.5.2.  path

   The path defines the allowed storage locations of the EBML Element
   within an EBML Document.  This path MUST be defined with the full
   hierarchy of EBML Elements separated with a "/".  The top EBML

Yet below the ABNF is using \. I think one of these need to be fixed.

 PathDelimiter            = "\"

This is supposed to be the same as the above, right?

 EBMLElementOccurrence    = [EBMLMinOccurrence] "*" [EBMLMaxOccurrence]
 EBMLMinOccurrence        = 1*DIGIT
 EBMLMaxOccurrence        = 1*DIGIT

Are there any upper limits on allowed values for these fields? Even if you don't encode them using ABNF, it would be good to mention them in an ABNF comment.

 VariableParentOccurrence = [PathMinOccurrence] "*" [PathMaxOccurrence]
 PathMinOccurrence        = 1*DIGIT
 PathMaxOccurrence        = 1*DIGIT

Same comment as above.

11.1.10.1.  label

   The label provides a concise expression for human consumption that
   describes what the value of the "<enum>" represents.

Is it worth adding a cross reference to the "lang" attribute here?

11.2.10.  DocTypeExtensionName Element

   description: The name of the DocTypeExtension to identify it from

I think the verb "differentiate" might be better than "identify" here.

   other DocTypeExtension of the same DocType+DocTypeVersion tuple.

12.  Considerations for Reading EBML Data

   If a Master Element contains a CRC-32 Element that doesn't validate,
   then the EBML Reader MAY ignore all contained data except for
   Descendant Elements that contain their own valid CRC-32 Element.

I don't fully understand your use of "MAY ... except ..." here.
Can you elaborate on why would an implementation ignore data contained in a Master Element and not ignore Descendant Elements, even if they own CRC-32 is valid?

   If a Master Element contains more occurrences of a Child Element that
   is not a Master Element than permitted according to the maxOccurs
   attribute of the definition of that Element then all but the instance
   of that Element with the smallest byte offset from the beginning of
   its Parent Element SHOULD be ignored.

I don't understand what this is is trying to say. If I have a Child Element with minOccurs = 2 and maxOccurs = 3, but the element appears 4 times: what would happen? I think the text above suggest to only use the first instance, which is possibly less than minOccurs.

16.  Security Considerations

   An EBML Reader MAY use the data if it considers it doesn't create any
   security issue.

I don't understand what this is trying to convey and how it can be complied with. Can you elaborate?
I suggest just deleting this sentence.

17.1.  CELLAR EBML Element ID Registry

   Four-octet Element IDs are numbers between 0x101FFFFF and 0x1FFFFFFE.
   Four-octet Element IDs are somewhat special in that they are useful
   for resynchronizing to major structures in the event of data
   corruption or loss.  As such four-octet Element IDs are split into
   two categories.  Four-octet Element IDs whose lower three octets (as
   encoded) would make printable 7-bit ASCII values (0x20 to 0x7F)

0x7F character is not considered printable. So I think this should be "(0x20 to 0x7E)". Ideally, you should also clarify that you want the whole range including 0x20 and 0x7E.

   MUST
   be allocated by the "Specification Required" policy.

17.2.  CELLAR EBML DocType Registry

   This document creates a new IANA Registry called "CELLAR EBML DocType
   Registry".

   DocType values are described in Section 11.1.3.1.  DocTypes are ASCII
   strings, defined in Section 7.4, which label the official name of the
   EBML Document Type.  The strings may be allocated according to the
   "First Come First Served" policy.

I think you need to be clearer on what you expect to appear on IANA's page. Do you expect a reference and/or description field in addition to a DocType name? I suspect you would also need the Change Controller field (who registered the value and can update the description).

   DocType string values of "matroska" and "webm" are RESERVED to the
   IETF for future use.  These can be assigned via the "IESG Approval"
   or "RFC Required" policies [RFC8126].

The last sentence is odd, but then I realized that you only reserve these two names and will attempt to assign them separately in the future. I suppose the current text is Ok.

Thank you,
Alexey

---

From: Dave Rice <[email-masked]>
Date: Sat, 6 Jul 2019 13:22:45 -0400

Hello Alexey,

Thanks much for providing this thorough review. I’ll try to reply point by point below with references to either pull requests or issues or offer followup questions. In a few places I ping Steve Lhomme and Michael Richardson as the comments relate to text they originated in their work.

> On Jul 3, 2019, at 9:44 AM, Alexey Melnikov <[email-masked]> wrote:
> 
> Hi all,

Added in https://github.com/cellar-wg/ebml-specification/pull/265 <https://github.com/cellar-wg/ebml-specification/pull/265>.

>   specific EBML Document Type.

Added to https://github.com/cellar-wg/ebml-specification/pull/265 <https://github.com/cellar-wg/ebml-specification/pull/265>.

> RFC 2119 MAY describes an implementation or deployment choice. So the above MAY doesn't seem to be appropriate. I suggest you change it to "can", as this is just a statement of fact.

Added to https://github.com/cellar-wg/ebml-specification/pull/265 <https://github.com/cellar-wg/ebml-specification/pull/265>.

> This is supposed to be the same as the above, right?

Good catch. Corrected to use \ in both places. Added to https://github.com/cellar-wg/ebml-specification/pull/265 <https://github.com/cellar-wg/ebml-specification/pull/265>.

> PathMaxOccurrence        = 1*DIGIT

I looked for examples of that sort of commenting but didn’t find much guidance. Eventually I simply appended " ; no upper limit” to each of the four referenced lines and added that to https://github.com/cellar-wg/ebml-specification/pull/265 <https://github.com/cellar-wg/ebml-specification/pull/265>.

> Is it worth adding a cross reference to the "lang" attribute here?

Do you mean to express the language of the term used within the label? Currently the language of the label is undefined and since it is an attribute that label is not repeatable.

>   other DocTypeExtension of the same DocType+DocTypeVersion tuple.

Added to https://github.com/cellar-wg/ebml-specification/pull/265 <https://github.com/cellar-wg/ebml-specification/pull/265>.

> Can you elaborate on why would an implementation ignore data contained in a Master Element and not ignore Descendant Elements, even if they own CRC-32 is valid?

For instance if a Matroska file has three metadata tags and each has a CRC value and so does the parent Tags element like this.

<Tags crc=invalid>
  <Tag crc=valid>
  <Tag crc=valid>
  <Tag crc=invalid>
<Tags>

We’re trying to say that even though the contents of the <Tags> element is invalid, that the valid child elements may still be used. An invalid descendant element would make all ancestor element invalid, but should not necessary make all sibling elements unusable.

> I don't understand what this is is trying to say. If I have a Child Element with minOccurs = 2 and maxOccurs = 3, but the element appears 4 times: what would happen? I think the text above suggest…

Good point, the language presumes that the minOccurs would be <= 1. This needs to be adjusted. Added an issue at https://github.com/cellar-wg/ebml-specification/issues/266 <https://github.com/cellar-wg/ebml-specification/issues/266>.

> I suggest just deleting this sentence.

Ping to Steve. This line was initially added in https://github.com/cellar-wg/ebml-specification/commit/640fbb71fb102f73948857bd97f7330d237bd5d6 <https://github.com/cellar-wg/ebml-specification/commit/640fbb71fb102f73948857bd97f7330d237bd5d6>. 
Issue added at https://github.com/cellar-wg/ebml-specification/issues/268 <https://github.com/cellar-wg/ebml-specification/issues/268>.

>   be allocated by the "Specification Required" policy.

Adjusted to say "0x20 to 0x7E, inclusive”. Added to https://github.com/cellar-wg/ebml-specification/pull/265 <https://github.com/cellar-wg/ebml-specification/pull/265>.

> The last sentence is odd, but then I realized that you only reserve these two names and will attempt to assign them separately in the future. I suppose the current text is Ok.

Ping to Michael Richardson and Steve. Any comment? Issue filed at https://github.com/cellar-wg/ebml-specification/issues/267 <https://github.com/cellar-wg/ebml-specification/issues/267>.

[…]

Thanks again for this helpful review.

Kind Regards,
Dave Rice

---

From: "Alexey Melnikov" <[email-masked]>
Date: Thu, 11 Jul 2019 14:36:09 +0100

Hi Dave,
I removed comments where we are in agreement. A few followups:

On Sat, Jul 6, 2019, at 6:23 PM, Dave Rice wrote:
> I looked for examples of that sort of commenting but didn’t find much guidance. Eventually I simply appended " ; no upper limit” to each of the four referenced lines and added that to https://github.com/cellar-wg/ebml-specification/pull/265.
I think this is the wrong fix. Is it sufficient for an implementation to use 32 bit value to represent any of these? 64 bit value?
"no upper limit" is not going to be interoperable.

> Do you mean to express the language of the term used within the label? Currently the language of the label is undefined and since it is an attribute that label is not repeatable.

To be honest I am not yet sure how I feel about "undefined language" here. Need to think about that.
But either way, I think adding some text that "lang" attribute doesn't apply would be helpful.

> We’re trying to say that even though the contents of the <Tags> element is invalid, that the valid child elements may still be used.
So to me this means that after discarding all invalid elements you end up with something like this:

<Tags >
 <Tag crc=valid>
</Tags>

As this is an incomplete document, I am struggling to understand what it can be used for?

>  An invalid descendant element would make all ancestor element invalid, but should not necessary make all sibling elements unusable.
> 

Best Regards,
Alexey

---

From: Dave Rice <[email-masked]>
Date: Thu, 11 Jul 2019 09:53:52 -0400

> On Jul 11, 2019, at 9:36 AM, Alexey Melnikov <[email-masked]> wrote:
> 
> Hi Dave,

The smallest possible EBML Element is 2 bytes (1 bytes Element ID, 1 byte Element Data Size, and 0 bytes of Element Data). The upper limit of how many occurrences would be determined by the limit of the Element Data Size of the Parent Element. If the EBML Document has EBMLMaxSizeLength as the default of 8, then the upper limit of an Element Data Size is 72,057,594,037,927,934 bytes. Is the smallest possible Element is 2 bytes, then the upper limit of that Element’s occurrence would be 72,057,594,037,927,934 divided by two.

So if EBMLMaxSizeLength = 8, then this is possible

<RootElement>
	<TwoByteElement/>  # 36,028,797,018,963,967 occurrences
</RootElement>

but this would overflow the Element Data Size of the Root Element
<RootElement>
	<TwoByteElement/>  # 36,028,797,018,963,968 occurrences
</RootElement>

However, EBML allows the EBML Document Type to set an EBMLMaxSizeLength value higher than 8, and as that is incremented up the upper limit would expand exponentially.

In my draft I would considering "no upper limit” and 36,028,797,018,963,967 to effectively be the same or that the definition does not define the limit but in practice it would be limited by the capacity of the Element Data Size of the parent Element.

> But either way, I think adding some text that "lang" attribute doesn't apply would be helpful.

In the case of Matroska, the labels are in English. In the EBML definition we could say that the labels are in English unless the definition of that associated EBML Document Type claims otherwise.

> As this is an incomplete document, I am struggling to understand what it can be used for?

In that case, the valid Tags are still useful even if within a parent element whos contents are invalid. Perhaps another example would be in the Attachments Element:

<Attachments crc=invalid>
	<Attachment crc=invalid>Poster Art</Attachment>
	<Attachment crc=valid>Subtitle Font</Attachment>
</Attachments>

Here some bit damage occurs to the Poster Art so the Attachment and its parent Attachments now have invalid crcs, however the subtitle font is still ok and could be used in the presentation without an issue.

> _______________________________________________
> Cellar mailing list
> [email-masked] <mailto:[email-masked]>
> https://www.ietf.org/mailman/listinfo/cellar <https://www.ietf.org/mailman/listinfo/cellar>

---

From: Michael Richardson <[email-masked]>
Date: Tue, 30 Jul 2019 16:25:58 -0400

> The last sentence is odd, but then I realized that you only reserve these two names and will attempt to assign them separately in the future. I suppose the current text is Ok.

Hi, we struggled with how we should initialize the DocType registry, 
since we expect "matroska" to be used by draft-ietf-cellar-matroska, and 
we expect "webm" to only be given to some new work (from google) that 
would define that format.

A decision as to who is the legitimate documentator of the (existing) 
"webm" DocType would be up to the IESG.

This text was suggested by IANA!