Closed Bug 852002 Opened 9 years ago Closed 9 years ago

nestegg loses parser sync after encountering zero length master element

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: kinetik, Assigned: kinetik)

References

()

Details

Attachments

(1 file)

Originally reported in bug 778436 comment 14.  Bug URL is a link to the attachment in bug 778436.

|+ Segment tracks at 288
| + A track at 300
|  + Track number: 1 (track ID for mkvmerge & mkvextract: 0) at 309
|  + Track type: audio at 312
|  + Track UID: 1009975861 at 315
|  + Audio track at 326
|  + Codec ID: A_VORBIS at 335
|  + CodecPrivate, length 2587 at 346
|  + Name: Audio at 2937

Note that there are no subelements of "Audio track at 326".  You'd usually see one or more of: SamplingFrequency, OutputSamplingFrequency, Channels.

The muxer has written an Audio element (0xE1) with a length of zero, the length is encoded as an 8 byte long uint (0x010000000000), but that's not a problem.  After reading that, nestegg expects subelements but does not encounter any, and loses parser sync.

0000140: 404b 3c33 0235 e101 0000 0000 0000 0086  @K<3.5..........
                        ^^ Audio
                          ^^ Size (0, encoded in 8 bytes)
                                              ^^ CodecID
0000150: 8941 5f56 4f52 4249 5300 63a2 4a1b 021e  .A_VORBIS.c.J...
         ^^ Size (9)
           ^^ ^^^^ ^^^^ ^^^^ ^^^^ CodecID string
                                  ^^ CodecPrivate
           ^^ nestegg parser syncs here after reading Audio element.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
The cause of this turns out to be simple.  For each element read, we peek the element ID and size, then prepare the parser based on the element ID, then read the element.

The code that allows peek and read to return the same element without rereading the same data offset is here: https://github.com/kinetiknz/nestegg/blob/master/src/nestegg.c#L835

  static int
  ne_peek_element(nestegg * ctx, uint64_t * id, uint64_t * size)
  {
    int r;

    if (ctx->last_id && ctx->last_size) {
      if (id)
        *id = ctx->last_id;
      if (size)
        *size = ctx->last_size;
      return 1;
    }
  [...]

Read calls peek, the resets the last_id and last_size to tell peek to read a new ID and size next time:

  static int
  ne_read_element(nestegg * ctx, uint64_t * id, uint64_t * size)
  {
    int r;

    r = ne_peek_element(ctx, id, size);
    if (r != 1)
      return r;

    ctx->last_id = 0;
    ctx->last_size = 0;
  [...]

The problem is that the Audio element's size is 0, so (ctx->last_id && ctx->last_size) fails as if the previous call was a read that cleared last_id and last_size.

This is the danger of trying to use a data value as a flag, when valid values overlap with values treated as a flag.  I'll post a patch shortly.
Attached patch patch v0Splinter Review
Introduce a dedicated flag for tracking peek/read state rather than overloading the values of last_id and last_size.
Attachment #726437 - Flags: review?(paul)
Attachment #726437 - Flags: review?(paul) → review+
Verified that the patch fixes my problem on fedora.

Also reviewed the code, looks good to me.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e41b1ed18c2

Thanks for the report, and confirming the fix!
https://hg.mozilla.org/mozilla-central/rev/0e41b1ed18c2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.