Closed Bug 1045909 Opened 5 years ago Closed 5 years ago

Fix buffered range estimation for MP4

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: ajones, Assigned: ajones)

References

Details

Attachments

(2 files)

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.
Attachment #8465916 - Flags: review?(edwin) → review+
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 :\)
Attachment #8466874 - Flags: review?(edwin) → review+
https://hg.mozilla.org/mozilla-central/rev/365ed105f9ce
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.