The current buffered ranges information only gives accurate information for the currently playing fragment. It needs to be able to parse all the buffered fragments whether or not we are playing them.

Comment on attachment 8465916 [details] [diff] [review] Fix buffer range calculation for fMP4 Review of attachment 8465916 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits. ::: media/libstagefright/binding/Box.cpp @@ +29,5 @@ > + > + if (mContext->mByteRanges[i].Contains(headerRange)) { > + break; > + } > + i++; Technically okay, but I find it odd that this isn't a for loop. @@ +40,5 @@ > + } > + > + uint32_t size = BigEndian::readUint32(header); > + if (size < 8) { > + return; size == 1 is valid, and means that there is a 64-bit length after the box tag. Yes, it's only meant for big boxes, but people are jerks. @@ +59,5 @@ > +bool > +Box::IsType(const char* aType) const > +{ > + MOZ_ASSERT(strlen(aType) == 4); > + return mType == BigEndian::readUint32(aType); This method is only ever called with constants. We should take a uint32_t here and do the fourcc conversion at compile time. @@ +73,5 @@ > +Box > +Box::FirstChild() const > +{ > + MOZ_ASSERT(IsAvailable()); > + return Box(mContext, Offset() + 8, this); Box headers can be >8 bytes long. I suggest we parse the whole header in the constructor and store the offset to the first child. ::: media/libstagefright/binding/DecoderData.cpp @@ +171,2 @@ > mime_type = aMimeType; > duration = FindInt64(aMetaData, kKeyDuration); |mime_type = ...| and |duration = ...| can be removed. ::: media/libstagefright/binding/MoofParser.cpp @@ +26,5 @@ > +MoofParser::RebuildFragmentedIndex(const nsTArray<MediaByteRange>& aByteRanges) > +{ > + BoxContext context(mSource, aByteRanges); > + > + mIndex.Clear(); For sufficiently long videos with sufficiently short fragments, I don't think we want to be doing all this work every time we update the seek bar. Is there a way that we can do this incrementally? (after talking on IRC, it sounds like optimisations are coming in a follow-up patch) ::: media/libstagefright/binding/include/mp4_demuxer/ByteReader.h @@ +22,5 @@ > ByteReader(const uint8_t* aData, size_t aSize) > : mPtr(aData), mRemaining(aSize) > { > } > + void Init(const nsTArray<uint8_t>& aData) Needs a more specific name. Generally a public method called `Init' implies that it should always be called. ::: media/libstagefright/binding/include/mp4_demuxer/MoofParser.h @@ +15,5 @@ > + > +class Tkhd > +{ > +public: > + Tkhd() : mCreationTime(0), mModificationTime(0), mTrackId(0), mDuration(0) {} please decide on... @@ +27,5 @@ > + > +class Mdhd > +{ > +public: > + Mdhd() : mCreationTime(0), mModificationTime(0), mTimescale(0), mDuration(0) ...a consistent... @@ +49,5 @@ > + , mDefaultSampleDuration(0) > + , mDefaultSampleSize(0) > + , mDefaultSampleFlags(0) > + { > + } ...ctor style. I don't care which, but IIRC this was the preferred style last time we had a dev-platform bikeshed over it.

Comment on attachment 8466874 [details] [diff] [review] imported patch _buffered_ranges_fixes.patch Review of attachment 8466874 [details] [diff] [review]: ----------------------------------------------------------------- Good. (I should have caught most of the MoofParser stuff in my first review :\)

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b1fb58e03f0d along with bug 1045915's backout for build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=45210935&tree=Mozilla-Inbound