Closed Bug 1202286 Opened 4 years ago Closed 4 years ago

Switch new MP3 demuxer to using a ByteReader for parsing file contents

Categories

(Core :: Audio/Video: Playback, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: JanH, Assigned: JanH, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(3 files, 9 obsolete files)

3.14 KB, patch
JanH
: review+
Details | Diff | Splinter Review
11.72 KB, patch
JanH
: review+
Details | Diff | Splinter Review
8.53 KB, patch
JanH
: review+
Details | Diff | Splinter Review
Currently, in FindNextFrame() the MP3 track demuxer manually reads a piece of the file into a buffer which is then passed to the MPEG frame parser. That in turn returns back a pointer pointing to the start of a MPEG audio frame if successful, or to the end of the buffer if nothing was found, in which case FindNextFrame() loads the next bits of data into that buffer and so forth.

As this is somewhat error-prone and vulnerable to potential over-/underflows if not handled carefully, it is preferable to have FindNextFrame() construct a ByteReader which can then be passed to the actual parsing functions to safely handle reading the data.
Assignee: nobody → jh+bugzilla
Sounds good.
Priority: -- → P2
Mentor: esawin
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [lang=c++]
Attachment #8666847 - Flags: review?(esawin)
Attachment #8666848 - Flags: feedback?(esawin)
Attachment #8666847 - Flags: review?(esawin) → review+
Comment on attachment 8666848 [details] [diff] [review]
Bug 1202286 - Part 1 - Switch MP3 demuxer to ByteReader.patch

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

That's a good base and looks semantically sound! Let's work out some of the comments before I continue with the feedback for the next patch.
You've done a good job at simplifying the Parse() functions, I think we can also reduce some of the complexity in FindNextFrame, too.

::: dom/media/MP3Demuxer.cpp
@@ +374,5 @@
>  
> +  FrameParserResult parseResult = { 0x0, 0 };
> +
> +  // Check whether we've found a valid MPEG frame.
> +  while (!(parseResult.mParseFlags & 0x1)) {

This is difficult to read and reason about, we should use enums or a boolean success state (see previous comment).

@@ +383,5 @@
>        break;
>      }
>      NS_ENSURE_TRUE(mOffset + read > mOffset, MediaByteRange(0, 0));
>      mOffset += read;
> +    mp4_demuxer::ByteReader reader = mp4_demuxer::ByteReader(buffer, read);

Please use |using mp4_demuxer::ByteReader| and reduce to |ByteReader reader(buffer, read)|.

@@ +385,5 @@
>      NS_ENSURE_TRUE(mOffset + read > mOffset, MediaByteRange(0, 0));
>      mOffset += read;
> +    mp4_demuxer::ByteReader reader = mp4_demuxer::ByteReader(buffer, read);
> +    parseResult = mParser.Parse(reader);
> +    reader.DiscardRemaining();

Don't discard yet, we need the offset in the case of success. In the case of no success, the reader should have consumed all remaining bytes, assert that.

@@ +390,3 @@
>  
> +    // Check whether we need to skip an ID3 tag which stretches beyond the current buffer.
> +    if (parseResult.mParseFlags & 0x2) {

The check is redundant.

@@ +396,3 @@
>    }
>  
> +  if (!(parseResult.mParseFlags & 0x1) || !mParser.CurrentFrame().Length()) {

See previous comment (bad readability).

@@ +591,5 @@
>  }
>  
>  FrameParserResult
> +FrameParser::Parse(mp4_demuxer::ByteReader& reader) {
> +  if (reader.Remaining() == 0) {

Use !reader.Remaining() instead. Also, this check should be redundant.

@@ +600,5 @@
>      // No MP3 frames have been parsed yet, look for ID3v2 headers at file begin.
>      // ID3v1 tags may only be at file end.
>      // TODO: should we try to read ID3 tags at end of file/mid-stream, too?
> +    const int32_t id3Offset = mID3Parser.Parse(reader);
> +    if (mID3Parser.Header().IsValid()) {

With the tag size returned and the reader state, we would have all the information we need without re-checking for header validity. This should also simplify the following statements.

@@ +611,5 @@
>          // return immediately and let the calling function handle skipping the rest of the tag.
>          MP3DEMUXER_LOGV("ID3v2 tag detected, size=%d, "
>                          "needing to skip %d bytes past the current buffer",
>                          tagSize, tagSize - remainingBuffer);
> +        return{ 0x2, (int32_t)(tagSize - remainingBuffer) };

We won't need casting here with my proposed changes, but if you have to cast, use C++-style casting (in this case static_cast).

@@ +616,4 @@
>        }
>        MP3DEMUXER_LOGV("ID3v2 tag detected, size=%d", tagSize);
> +      MOZ_ASSERT(id3Offset + tagSize < reader.Length());
> +      reader.Seek(id3Offset + tagSize);

Use Remaining() and Read() instead.

@@ +623,4 @@
>      }
>    }
>  
> +  while (reader.Remaining() > 0 && !mFrame.ParseNext(reader.ReadU8())) { }

Use CanRead8() instead.

@@ +630,5 @@
>      if (!mFirstFrame.Length()) {
>        mFirstFrame = mFrame;
>      }
> +    // Return the frame header begin to allow for whole-frame parsing.
> +    return{ 0x1, (int32_t)(reader.Offset() - FrameHeader::SIZE) };

See above. Also consider using Rewind().

@@ +957,5 @@
>  } // namespace id3_header
>  
> +int32_t
> +ID3Parser::Parse(mp4_demuxer::ByteReader& reader) {
> +  if (reader.Remaining() == 0) {

See above.

@@ +963,3 @@
>    }
>  
> +  while (reader.Remaining() > 0 && !mHeader.ParseNext(reader.ReadU8())) { }

Use CanRead8() instead.

@@ +965,5 @@
> +  while (reader.Remaining() > 0 && !mHeader.ParseNext(reader.ReadU8())) { }
> +
> +  if (mHeader.IsValid()) {
> +    // Header found, return offset relative to parsed buffer.
> +    return reader.Offset() - ID3Header::SIZE;

Let's return the tag size instead, this will make this function more generally applicable.

::: dom/media/MP3Demuxer.h
@@ +7,5 @@
>  
>  #include "mozilla/Attributes.h"
>  #include "MediaDataDemuxer.h"
>  #include "MediaResource.h"
> +#include "mp4_demuxer/ByteReader.h"

We should think about a follow-up to move this out of mp4_demuxer.

@@ +102,5 @@
> +  // Parses contents of given ByteReader for a valid ID3 header.
> +  // If successful, returns the header offset relative to the start of the
> +  // parsed buffer.
> +  // Note: To check for success, check the ID3 header for validity afterwards.
> +  int32_t Parse(mp4_demuxer::ByteReader& reader);

It would be nice to have the result in the return value instead of having to check external state. Why not just return the tag size and 0 if no tag has been identified? With the tag size, we can deduce the skip size (tag size - static header size).

@@ +116,5 @@
> +// mParseFlags: 0x0 = neither MPEG frame header nor ID3 tag detected
> +//              0x1 = MPEG frame header detected, mParseOffset indicates offset
> +//                    relative to buffer start
> +//              0x2 = ID3 tag detected, mParseOffset indicates amount of additional
> +//                    bytes to skip

Please use either an enum or a bool for this. Having an enum could be more descriptive, but it shouldn't be necessary, since we don't need to distinguish between 0x0 and 0x2 having a dedicated bytes-to-skip return value.

@@ +121,3 @@
>  struct FrameParserResult {
> +  uint8_t mParseFlags;
> +  int32_t mParseOffset;

ByteReader keeps the offset for us, this shouldn't be required anymore.
The result should just contain the success state (bool or enum) and bytes to skip.

@@ +297,5 @@
>    // - resets ID3Header if no valid header was parsed yet
>    void EndFrameSession();
>  
> +  // Parses contents of given ByteReader for a valid frame header and returns a FrameParserResult.
> +  FrameParserResult Parse(mp4_demuxer::ByteReader& reader);

We should update the return value description or add a comment to the struct definition.

::: media/libstagefright/binding/include/mp4_demuxer/ByteReader.h
@@ +54,5 @@
> +  size_t Length()
> +  {
> +    return mLength;
> +  }
> +

This shouldn't be required, we have Remaining() and Offset() to show us the position within the "byte-slice".
Attachment #8666848 - Flags: feedback?(esawin) → feedback+
I've just noticed that the order of my comments is not as expected, sorry about the confusion of the "see above" comments, which might refer to a comment "below".
Now also fixing a small typo I found in a comment. Carrying forward r+ from esawin.
Attachment #8666847 - Attachment is obsolete: true
Attachment #8668400 - Flags: review+
Okay, I hope I have addressed all of your comments and haven't forgotten anything.

I've eliminated the struct and as discussed yesterday I'm now passing a boolean variable by reference to FrameParser::Parse, which after returning contains the success status of the parsing attempt.

> @@ +385,5 @@
> >      NS_ENSURE_TRUE(mOffset + read > mOffset, MediaByteRange(0, 0));
> >      mOffset += read;
> > +    mp4_demuxer::ByteReader reader = mp4_demuxer::ByteReader(buffer, read);
> > +    parseResult = mParser.Parse(reader);
> > +    reader.DiscardRemaining();
> 
> Don't discard yet, we need the offset in the case of success. In the case of
> no success, the reader should have consumed all remaining bytes, assert that.

I'm now capturing the (potential) frame header offset into a variable before the reader goes out of scope.

> @@ +390,3 @@
> >  
> > +    // Check whether we need to skip an ID3 tag which stretches beyond the current buffer.
> > +    if (parseResult.mParseFlags & 0x2) {
> 
> The check is redundant.

I've realised this myself, fixed in the new implementation as well.

> @@ +591,5 @@
> >  }
> >  
> >  FrameParserResult
> > +FrameParser::Parse(mp4_demuxer::ByteReader& reader) {
> > +  if (reader.Remaining() == 0) {
> 
> Use !reader.Remaining() instead. Also, this check should be redundant.

It was a direct replacement of the original parameter checking, but you're right, in all instances of this pattern we check the remaining length of the reader again during parsing, so if the reader really is empty, we'll fall through to returning whatever the appropriate failure value is anyway.

> @@ +600,5 @@
> >      // No MP3 frames have been parsed yet, look for ID3v2 headers at file begin.
> >      // ID3v1 tags may only be at file end.
> >      // TODO: should we try to read ID3 tags at end of file/mid-stream, too?
> > +    const int32_t id3Offset = mID3Parser.Parse(reader);
> > +    if (mID3Parser.Header().IsValid()) {
> 
> With the tag size returned and the reader state, we would have all the
> information we need without re-checking for header validity. This should
> also simplify the following statements.

Implemented - ID3 parser now returns the total tag size if successful, with the subsequent calculations reworked.

> @@ +630,5 @@
> >      if (!mFirstFrame.Length()) {
> >        mFirstFrame = mFrame;
> >      }
> > +    // Return the frame header begin to allow for whole-frame parsing.
> > +    return{ 0x1, (int32_t)(reader.Offset() - FrameHeader::SIZE) };
> 
> See above. Also consider using Rewind().

The header to be parsed might lie across a buffer boundary, in which case after having detected a complete header we'd have to rewind the reader to a negative value, which isn't possible. So we just leave the reader where it is and subtract FrameHeader::SIZE after having returned to FindNextFrame().

> ::: media/libstagefright/binding/include/mp4_demuxer/ByteReader.h
> @@ +54,5 @@
> > +  size_t Length()
> > +  {
> > +    return mLength;
> > +  }
> > +
> 
> This shouldn't be required, we have Remaining() and Offset() to show us the
> position within the "byte-slice".

I originally found this easier than having to write |reader.Offset() + reader.Remaining()|, but after a bit of rewriting I indeed don't need this anymore, so I've eliminated it again.
Attachment #8666848 - Attachment is obsolete: true
Attachment #8668463 - Flags: review?(esawin)
Rebased, plus applicable fixes from the feedback for part 1 and our discussion.
Attachment #8666849 - Attachment is obsolete: true
Attachment #8666849 - Flags: feedback?(esawin)
Attachment #8668465 - Flags: review?(esawin)
Comment on attachment 8668463 [details] [diff] [review]
Bug 1202286 - Part 1v2 - Switch MP3 demuxer to ByteReader.patch

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

This looks good, but let's address the comments before the final review.

::: dom/media/MP3Demuxer.cpp
@@ +375,2 @@
>  
> +  bool parseSuccess = false;

Maybe change that to something more descriptive like foundFrame?

@@ +384,5 @@
>        // This is not a valid MPEG audio stream or we've reached EOS, give up.
>        break;
>      }
>      NS_ENSURE_TRUE(mOffset + read > mOffset, MediaByteRange(0, 0));
>      mOffset += read;

We should be able to move this to the end of the scope, reducing the number of statements and simplifying the frame header offset calculation.

@@ +385,5 @@
>        break;
>      }
>      NS_ENSURE_TRUE(mOffset + read > mOffset, MediaByteRange(0, 0));
>      mOffset += read;
> +    ByteReader reader = ByteReader(buffer, read);

ByteReader reader(buffer, read); should work.

@@ +387,5 @@
>      NS_ENSURE_TRUE(mOffset + read > mOffset, MediaByteRange(0, 0));
>      mOffset += read;
> +    ByteReader reader = ByteReader(buffer, read);
> +    const uint32_t bytesToSkip = mParser.Parse(&reader, &parseSuccess);
> +    frameHeaderOffset = reader.Offset() - FrameParser::FrameHeader::SIZE;

We can make this offset stream-relative instead of relative to the reader buffer. This should make nextBeg redundant. Make sure to adjust the variable type.

@@ +392,5 @@
>  
> +    // If we haven't found neither an MPEG frame header nor an ID3v2 tag,
> +    // the reader shouldn't have any bytes remaining.
> +    MOZ_ASSERT(parseSuccess || !parseSuccess && bytesToSkip > 0 ||
> +               !parseSuccess && !bytesToSkip && !reader.Remaining());

Due to lazy evaluation, this can be reduced to MOZ_ASSERT(parseSuccess || bytesToSkip || !reader.Remaining());

@@ +608,2 @@
>        // ID3 tag found, skip past it.
> +      const uint32_t remainingBuffer = aReader->Remaining() + ID3Parser::ID3Header::SIZE;

The variable name is misleading, that's the complete ID3 tag (+ header) size, not the remaining buffer. It might be clearer to find the skipSize (= tagSize - ID3Header::SIZE) instead and work with that.

@@ +619,3 @@
>        }
>        MP3DEMUXER_LOGV("ID3v2 tag detected, size=%d", tagSize);
> +      MOZ_ASSERT(tagSize - ID3Parser::ID3Header::SIZE <= aReader->Remaining());

Here it is used, the skipSize.

@@ +619,4 @@
>        }
>        MP3DEMUXER_LOGV("ID3v2 tag detected, size=%d", tagSize);
> +      MOZ_ASSERT(tagSize - ID3Parser::ID3Header::SIZE <= aReader->Remaining());
> +      aReader->Read(tagSize - ID3Parser::ID3Header::SIZE);

And again.

@@ +621,5 @@
> +      MOZ_ASSERT(tagSize - ID3Parser::ID3Header::SIZE <= aReader->Remaining());
> +      aReader->Read(tagSize - ID3Parser::ID3Header::SIZE);
> +    } else {
> +      // No ID3 tag found, rewinding reader in order to search for a MPEG frame header.
> +      aReader->Reset();

That would reset the reader offset to 0, which might not be the offset we have started with when this parse function was called. It's not a big issue with small reader buffers, but we should only rewind the number of bytes we have parsed during the ID3 search.

@@ +968,5 @@
> +  while (aReader->CanRead8() && !mHeader.ParseNext(aReader->ReadU8())) { }
> +
> +  if (mHeader.IsValid()) {
> +    // Header found, return total tag size.
> +    return ID3Parser::ID3Header::SIZE + Header().Size() + Header().FooterSize();

ID3Parser:: is redundant, that's the current scope.

::: dom/media/MP3Demuxer.h
@@ +289,5 @@
> +  // After returning, the variable passed to 'success' indicates whether
> +  // parsing was successful.
> +  // The return value indicates the amount of bytes to be skipped (if any)
> +  // in order to jump across a large ID3 tag spanning multiple buffers.
> +  uint32_t Parse(mp4_demuxer::ByteReader* aReader, bool* aSuccess);

Let's swap this around: return success and pass bytes-to-skip as pointer instead. That would be more consistent with the code base and could make the call site more intuitive.
Attachment #8668463 - Flags: review?(esawin) → feedback+
Suggested changes implemented.

> @@ +392,5 @@
> >  
> > +    // If we haven't found neither an MPEG frame header nor an ID3v2 tag,
> > +    // the reader shouldn't have any bytes remaining.
> > +    MOZ_ASSERT(parseSuccess || !parseSuccess && bytesToSkip > 0 ||
> > +               !parseSuccess && !bytesToSkip && !reader.Remaining());
> 
> Due to lazy evaluation, this can be reduced to MOZ_ASSERT(parseSuccess ||
> bytesToSkip || !reader.Remaining());

Now you're saying it, it looks obvious, but thanks for pointing it out.

> @@ +608,2 @@
> >        // ID3 tag found, skip past it.
> > +      const uint32_t remainingBuffer = aReader->Remaining() + ID3Parser::ID3Header::SIZE;
> 
> The variable name is misleading, that's the complete ID3 tag (+ header)
> size, not the remaining buffer. It might be clearer to find the skipSize (=
> tagSize - ID3Header::SIZE) instead and work with that.

In a way it is the remaining buffer size - it's the distance between the beginning of the ID3 tag and the end of the current parsing buffer. But the skipSize idea looks a bit better to me as well, so I've changed it.

> @@ +621,5 @@
> > +      MOZ_ASSERT(tagSize - ID3Parser::ID3Header::SIZE <= aReader->Remaining());
> > +      aReader->Read(tagSize - ID3Parser::ID3Header::SIZE);
> > +    } else {
> > +      // No ID3 tag found, rewinding reader in order to search for a MPEG frame header.
> > +      aReader->Reset();
> 
> That would reset the reader offset to 0, which might not be the offset we
> have started with when this parse function was called. It's not a big issue
> with small reader buffers, but we should only rewind the number of bytes we
> have parsed during the ID3 search.

Currently the function is always called with a fresh ByteReader, so it doesn't matter, although maybe in that case I should have made that assumption explicit by calling aReader->Reset before actually using it. However in any case I've implemented your suggestion for future-proofing.
Attachment #8668463 - Attachment is obsolete: true
Attachment #8669138 - Flags: review?(esawin)
Rebased for the changes in Part 1.
Attachment #8668465 - Attachment is obsolete: true
Attachment #8668465 - Flags: review?(esawin)
Attachment #8669139 - Flags: review?(esawin)
Comment on attachment 8669138 [details] [diff] [review]
Bug 1202286 - Part 1v3 - Switch MP3 demuxer to ByteReader.patch

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

This simplifies the parsing and makes it somewhat safer, great work!
r=me with the nits addressed.

::: dom/media/MP3Demuxer.cpp
@@ +375,3 @@
>  
> +  bool foundFrame = false;
> +  int64_t frameHeaderOffset;

Please initialize to 0.

@@ +387,3 @@
>  
> +    ByteReader reader(buffer, read);
> +    uint32_t bytesToSkip;

= 0.

@@ +602,5 @@
>    if (!mID3Parser.Header().Size() && !mFirstFrame.Length()) {
>      // No MP3 frames have been parsed yet, look for ID3v2 headers at file begin.
>      // ID3v1 tags may only be at file end.
>      // TODO: should we try to read ID3 tags at end of file/mid-stream, too?
> +    const size_t previousReaderOffset = aReader->Offset();

nit: Please shorten variable name to prevReaderOffset.

@@ +633,5 @@
>      if (!mFirstFrame.Length()) {
>        mFirstFrame = mFrame;
>      }
> +    // Indicate success.
> +    *aBytesToSkip = 0;

Let's just initialize it with 0 at function begin so we can remove these statements.
Attachment #8669138 - Flags: review?(esawin) → review+
Comment on attachment 8669139 [details] [diff] [review]
Bug 1202286 - Part 2v3 - Switch VBR header parsing to ByteReader as well.patch

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

I like how this improves code readability.
The only issue I have is that I would prefer the call site to control the starting reader offset, the parse functions should work only on the remaining buffer relative to the offset at function start.
This would allow us to pass much bigger reader buffers in future with longer persistence and it improves generality without introducing much complexity.

::: dom/media/MP3Demuxer.cpp
@@ +470,5 @@
>  
>    if (mNumParsedFrames == 1) {
>      // First frame parsed, let's read VBR info if available.
>      // TODO: read info that helps with seeking (bug 1163667).
> +    ByteReader reader = ByteReader(frame->Data(), frame->Size());

ByteReader reader(frame->Data(), frame->Size());

@@ +851,5 @@
>  
>  bool
> +FrameParser::VBRHeader::ParseXing(ByteReader* aReader) {
> +  static const uint32_t TAG1 = BigEndian::readUint32("Xing");
> +  static const uint32_t TAG2 = BigEndian::readUint32("Info");

We probably shouldn't mix this into a refactoring patch to make potential regression tracking and uplifts easier.

@@ +863,5 @@
>      VBR_SCALE = 0x08
>    };
>  
> +  MOZ_ASSERT(aReader);
> +  aReader->Reset();

Why resetting? I would prefer that the call site has control over the starting reader offset, if that's possible.

@@ +873,4 @@
>        continue;
>      }
>  
> +    aReader->Read(sizeof(TAG1));

Why not ReadU32()? Please add a comment that it's  for tag skipping.

@@ +875,5 @@
>  
> +    aReader->Read(sizeof(TAG1));
> +    const uint32_t flags = aReader->ReadU32();
> +    if (flags & NUM_FRAMES) {
> +      mNumFrames = aReader->ReadU32();

This will cause an assertion to fail iff !aReader->CanRead32(). We still need the check.

@@ +891,5 @@
>    static const uint32_t FRAME_COUNT_OFFSET = OFFSET + 14;
>    static const uint32_t MIN_FRAME_SIZE = OFFSET + 26;
>  
> +  MOZ_ASSERT(aReader);
> +  aReader->Reset();

Again, don't really like it unless there is a good reason for it.

@@ +897,3 @@
>    // VBRI have a fixed relative position, so let's check for it there.
> +  if (aReader->Remaining() > MIN_FRAME_SIZE) {
> +    aReader->Seek(OFFSET);

Without the Reset(), we shouldn't make the assumption of having started at offset 0, use Read() instead or make it relative to the original reader offset at function start.

@@ +897,5 @@
>    // VBRI have a fixed relative position, so let's check for it there.
> +  if (aReader->Remaining() > MIN_FRAME_SIZE) {
> +    aReader->Seek(OFFSET);
> +    if (aReader->ReadU32() == TAG) {
> +      aReader->Seek(FRAME_COUNT_OFFSET);

Same.
Attachment #8669139 - Flags: review?(esawin) → feedback+
Rebased onto a more current m-c, no changes otherwise. Carrying forward r+ from esawin.
Attachment #8668400 - Attachment is obsolete: true
Attachment #8669710 - Flags: review+
Nits addressed, carrying forward r+ from esawin.
Attachment #8669138 - Attachment is obsolete: true
Attachment #8669712 - Flags: review+
> The only issue I have is that I would prefer the call site to control the
> starting reader offset, the parse functions should work only on the
> remaining buffer relative to the offset at function start.
> This would allow us to pass much bigger reader buffers in future with longer
> persistence and it improves generality without introducing much complexity.

Implemented, the VBR header parsing functions now preserve the initial ByteReader offset. As the VBRI parsing function is still expecting to start at the beginning of an MPEG frame, I've also made that assumption more explicit.

> @@ +851,5 @@
> >  
> >  bool
> > +FrameParser::VBRHeader::ParseXing(ByteReader* aReader) {
> > +  static const uint32_t TAG1 = BigEndian::readUint32("Xing");
> > +  static const uint32_t TAG2 = BigEndian::readUint32("Info");
> 
> We probably shouldn't mix this into a refactoring patch to make potential
> regression tracking and uplifts easier.

As per our discussion on IRC, one line this change is touching is also touched by the ByteReader refactoring, so a separate patch wouldn't be completely independent. As it's a small change, I've therefore kept it as part of this patch.
 
> @@ +873,4 @@
> >        continue;
> >      }
> >  
> > +    aReader->Read(sizeof(TAG1));
> 
> Why not ReadU32()? Please add a comment that it's  for tag skipping.

To make it more explicit that we want to skip exactly the size of the tag, even if this is effectively the same as using ReadU32(). However I've added a comment as well.

> @@ +875,5 @@
> >  
> > +    aReader->Read(sizeof(TAG1));
> > +    const uint32_t flags = aReader->ReadU32();
> > +    if (flags & NUM_FRAMES) {
> > +      mNumFrames = aReader->ReadU32();
> 
> This will cause an assertion to fail iff !aReader->CanRead32(). We still
> need the check.

The while loop checks that we have at least FRAME_COUNT_OFFSET + FRAME_COUNT_SIZE remaining, i.e. 12 bytes.
If parsing was unsuccessful, we simply advance one byte and run the loop again.
If on the other hand parsing was successful, we end up reading at most exactly those 12 bytes: one Read(sizeof(TAG)), i.e. 4 bytes, for the tag, one ReadU32() for reading the flags and finally if it exists, another ReadU32() for reading the frame count itself.
Attachment #8669139 - Attachment is obsolete: true
Attachment #8669727 - Flags: review?(esawin)
Comment on attachment 8669727 [details] [diff] [review]
Bug 1202286 - Part 2v4 - Switch VBR header parsing to ByteReader as well.patch

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

Looks good!

::: dom/media/MP3Demuxer.cpp
@@ +877,5 @@
>        continue;
>      }
> +    // Skip across the VBR header ID tag.
> +    aReader->Read(sizeof(TAG));
> +    

Whitespace.
Attachment #8669727 - Flags: review?(esawin) → review+
Whitespace removed, carrying forward r+ from esawin.
Attachment #8669727 - Attachment is obsolete: true
Attachment #8669828 - Flags: review+
Keywords: checkin-needed
Depends on: 1263334
You need to log in before you can comment on or make changes to this bug.