782457 - DASH: Add nestegg support for specifying byte ranges of init and cues metadata

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Steve Workman [:sworkman] (INACTIVE)]

For the DASH-WebM On Demand profile, it is necessary to preload initialization information and cues metadata before starting cluster downloads. This requires some changes in nestegg to deal with specific init and cues byte ranges.

Background: In this DASH profile, stream switching is supposed to happen between clusters. Cluster offsets are not provided in the DASH manifest, but are instead to be read from the video files themselves.
-- Note that byte ranges for init and cues data ARE to be supplied in the DASH manifest, allowing client apps to seek and read the relevant bytes.

http://wiki.webmproject.org/adaptive-streaming/webm-dash-specification

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: Add nestegg support for specifying byte ranges of init and cues metadata

Changes libnestegg to deal with max offsets and to allow for specific loading of cues data into the nestegg context structure.

See function comments in nestegg.h for more information.

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: Sample patch to collect cues data in nsWebMReader

Presents a sample patch to collect cues data in nsWebMReader.

Adjusts nsWebMReader so that byte ranges can be specified for init and cues metadata. Then, in ReadMetadata, nestegg is told not to go past the max offset for init metadata, and to read cues data into the nestegg context struct if the relevant byte ranges have been specified.

Note: this sample usage is from a work-in-progress patch for DASH.

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 651565 [details] [diff] [review]
Add nestegg support for specifying byte ranges of init and cues metadata

Changing to r? from f? as I want to get this landed soon, not just discuss the approach. Of course, if the approach is wrong/undesirable, I want to hear about that too :)

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 651565 [details] [diff] [review]
Add nestegg support for specifying byte ranges of init and cues metadata

Review of attachment 651565 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not entirely sure I understand what's required for the max_offset part of this.  Is it supposed to be a precise hard limit of the read offset, or just a hint?

The existing implementation pauses parsing and returns from nestegg_init once the first Cluster is seen, parsing the Cues if they were present before that point.  Otherwise, they're loaded lazily from the offset specified in the SeekHead the first time a seek is attempted.  Typically the Cues are at the start or end of the file.  What's expected for DASH?  Presumably for the initialization segment you need to pause parsing (just) before the first Cluster (which will be in a different segment)?

::: media/libnestegg/src/nestegg.c
@@ +967,4 @@
>  {
>    int r;
>    int64_t * data_offset;
> +  uint64_t id, size, elem_num = 0;

Unused variable.

@@ +983,4 @@
>      return -1;
>  
>    for (;;) {
> +    if ((max_offset > 0) && (ne_io_tell(ctx->io) >= max_offset)) {

This is over-parenthesized.

@@ +1428,2 @@
>      return -1;
> +  }

No braces for single line conditionals.

@@ +1543,5 @@
>  {
>    int r;
> +  struct ebml_list_node * node = ctx->segment.cues.cue_point.head;
> +  struct seek * found;
> +  uint64_t seek_pos, id;

These three variables seem to be unused.

@@ +1548,3 @@
>    struct saved_state state;
> +  
> +  

Double newline.

@@ +1594,5 @@
>      if (r < 0)
>        return -1;
> +
> +    node = ctx->segment.cues.cue_point.head;
> +    // Final check before returning

C89 style comments only please.  But this comment seems obvious enough to remove.

@@ +1611,5 @@
> +  struct cue_point * cue_point;
> +  struct cue_track_positions * pos;
> +  uint64_t seek_pos, first_seek_pos, tc_scale, t, id;
> +  struct ebml_list_node * node = ctx->segment.cues.cue_point.head;
> +  struct ebml_list_node * node2 = NULL;

Since these are walking two different lists they need more precise names.  cue and trackpos, maybe?

@@ +1621,5 @@
> +  /* Initialise return values */
> +  *start_pos = -1;
> +  *end_pos   = -1;
> +
> +  if (!node) {

Originally I was going to say: I think this should just return an error rather than calling init.  Leave it up to the caller to call init before calling get.  That also means you don't need to pass max_offset into this function.

But I think it'd be nicer if init was an internal only function and get and seek called it as needed.

@@ +1632,5 @@
> +
> +  nestegg_track_count(ctx, &track_count);
> +
> +  while (node && !range_obtained) {
> +    if (node->id == ID_CUE_POINT) {

This can be an assert, something has gone very wrong if the cue points list contains anything but cue points.

@@ +1636,5 @@
> +    if (node->id == ID_CUE_POINT) {
> +      cue_point = node->data;
> +      node2 = cue_point->cue_track_positions.head;
> +      while (node2) {
> +        pos = node2->data;

assert node2->id is ID_CUE_TRACK_POSITIONS.

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2 Add nestegg support for specifying byte ranges of init and cues metadata

Thanks for the review, Matthew!

(In reply to Matthew Gregan [:kinetik] from comment #4)
> I'm not entirely sure I understand what's required for the max_offset part
> of this.  Is it supposed to be a precise hard limit of the read offset, or
> just a hint?

Hard limit (See background below).
 
> The existing implementation pauses parsing and returns from nestegg_init
> once the first Cluster is seen, parsing the Cues if they were present before
> that point.  Otherwise, they're loaded lazily from the offset specified in
> the SeekHead the first time a seek is attempted.  Typically the Cues are at
> the start or end of the file.  What's expected for DASH?  Presumably for the
> initialization segment you need to pause parsing (just) before the first
> Cluster (which will be in a different segment)?

The background is that the DASH manifest will provide an init range and a cues range, both of which should be downloaded first. Also, the cues range is used to create HTTP byte range requests to download clusters/groups of clusters per request. We can't stop at the start of a cluster if the cluster bytes have not been downloaded yet. Hence, forcing nestegg to stop at the end offset specified in the manifest.

> ::: media/libnestegg/src/nestegg.c
> @@ +967,4 @@
> […]
> > +  uint64_t id, size, elem_num = 0;
> 
> Unused variable.

Oops - removed.

> @@ +983,4 @@
> [...]
> > +    if ((max_offset > 0) && (ne_io_tell(ctx->io) >= max_offset)) {
> 
> This is over-parenthesized.

Fixed.

> @@ +1428,2 @@
> […]
> > +  }
> 
> No braces for single line conditionals.

Fixed. Roc told me to add these in other media files :)

> @@ +1543,5 @@
> […]
> > +  struct ebml_list_node * node = ctx->segment.cues.cue_point.head;
> > +  struct seek * found;
> > +  uint64_t seek_pos, id;
> 
> These three variables seem to be unused.

I see them used in the function.

> @@ +1548,3 @@
> […]  
> Double newline.

Removed.

> @@ +1594,5 @@
> […]
> C89 style comments only please.  But this comment seems obvious enough to
> remove.

Removed this one; fixed another.

> @@ +1611,5 @@
> […]
> > +  struct ebml_list_node * node = ctx->segment.cues.cue_point.head;
> > +  struct ebml_list_node * node2 = NULL;
> 
> Since these are walking two different lists they need more precise names. 
> cue and trackpos, maybe?

Fixed: now they are cues_node and cue_pos_node.

> @@ +1621,5 @@
> […]
> Originally I was going to say: I think this should just return an error
> rather than calling init.  Leave it up to the caller to call init before
> calling get.  That also means you don't need to pass max_offset into this
> function.
> 
> But I think it'd be nicer if init was an internal only function and get and
> seek called it as needed.

Nice. Done. Removed reference from nsWebMReader.cpp in future patch, and removed symbol from symbols.def.in for Win builds. Note: I moved the comment for the function from nestegg.h to nestegg.c - you can tell me if you think it's not needed.

> @@ +1632,5 @@
> […]
> > +    if (node->id == ID_CUE_POINT) {
> 
> This can be an assert, something has gone very wrong if the cue points list
> contains anything but cue points.

Done.

> @@ +1636,5 @@
> […]
> > +      while (node2) {
> > +        pos = node2->data;
> 
> assert node2->id is ID_CUE_TRACK_POSITIONS.

Done.

Thanks again! New patch should be uploaded and ready for second review.

Comment 6

[Matthew Gregan [:kinetik]]

Comment on attachment 659933 [details] [diff] [review]
v2 Add nestegg support for specifying byte ranges of init and cues metadata

Review of attachment 659933 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  Sorry for the delay on the second review, I've been out with a cold.

::: media/libnestegg/src/nestegg.c
@@ +982,5 @@
>    if (!ctx->ancestor)
>      return -1;
>  
>    for (;;) {
> +    if (max_offset > 0 && ne_io_tell(ctx->io) >= max_offset) {

I suspect it's acceptable (at least, for now) so I'm only pointing this out to make sure it's clear to both of us: bailing from the parser based on the specified offset from this loop will only work for "well formed" file and offset pairs, because the parser may read multiple bytes before returning to this loop.  For a non-"well formed" file/offset pair, this check this check will result in the file being treated as corrupt.

If we need to extend this in the future, I think changing the IO level to allow returning a temporary failure error (e.g. EINTR or something) and fixing the appropriate bits of the parser to allow suspending at arbitrary byte offsets when IO returns EINTR is the way to go.

@@ +1545,2 @@
>  int
> +nestegg_init_cue_points(nestegg * ctx, int64_t max_offset)

Please mark this function static, move it above the first external function (nestegg_init), and rename it to ne_init_cue_points.  (ne_ is for internal functions, anything named nestegg_* is marked as an exported symbol in the upstream build system).  I haven't bothered to comment the other internal functions, so I'd be happy to drop the comment on this one too.

@@ +1634,5 @@
> +
> +  nestegg_track_count(ctx, &track_count);
> +
> +  while (cues_node && !range_obtained) {
> +  assert(cues_node->id == ID_CUE_POINT);

Indentation is off here.

Comment 7

[Matthew Gregan [:kinetik]]

Forgot to mention: for getting this checked in, the best way is to apply the changes upstream (https://github.com/kinetiknz/nestegg) and then use media/libnestegg/update.sh to import them so that README_MOZILLA tracks the commit ID correctly.  If you've got a github account and don't mind, submit a pull request (and add yourself to AUTHORS), otherwise let me know and I'll integrate the patch upstream.

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

Pull request on github: https://github.com/kinetiknz/nestegg/pull/2

I added the following as a comment there: "Matthew, I noticed that the most recent patch uploaded to Mozilla looks to be Dec 16, 2010, (https://github.com/kinetiknz/nestegg/commit/ef45a1c3a6b86ce3d86d179537e94c57008f07f4), but there are patches since then. As such, I de-bitrotted the patch that was uploaded to bugzilla, and it means we'll be brining in some other patches with the DASH one. Hope that's ok with you."

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

Oh, and thanks for the r+ :)

Comment 10

[Matthew Gregan [:kinetik]]

(In reply to Steve Workman [:sworkman] from comment #8)
> I added the following as a comment there: "Matthew, I noticed that the most
> recent patch uploaded to Mozilla looks to be Dec 16, 2010,
> (https://github.com/kinetiknz/nestegg/commit/
> ef45a1c3a6b86ce3d86d179537e94c57008f07f4), but there are patches since then.
> As such, I de-bitrotted the patch that was uploaded to bugzilla, and it
> means we'll be brining in some other patches with the DASH one. Hope that's
> ok with you."

Yeah, that's intended, thanks.  This way the upstream version and the version in mozilla-central remain in sync.  I was surprised it was so long since the last import, so I reviewed the changes.  The only actual code change is https://github.com/kinetiknz/nestegg/commit/114397a15829f552f572fe30baca516c7454aaab, which is obviously trivial.

Thanks for the patch!

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

Thanks, Matthew. FYI, I'm aiming to land a few patches together at the start of next week, including this one. So, don't worry if you don't see an inbound link in a comment until then :)

Comment 12

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 659933 [details] [diff] [review]
v2 Add nestegg support for specifying byte ranges of init and cues metadata

https://hg.mozilla.org/integration/mozilla-inbound/rev/c72351cdf9dc

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

Try job for patch landed as per comment 12.
https://tbpl.mozilla.org/?tree=Try&rev=89ac34c1d14b

Comment 14

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/c72351cdf9dc