Make MP4Reader::GetBuffered() estimation accurate MP4

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kentuckyfriedtakahe, Assigned: kentuckyfriedtakahe)

Tracking

Trunk
mozilla36
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

The current buffered estimation is an approximation based on file size versus buffered bytes. We need to use the data from the sample table to get accurate values.
Created attachment 8450786 [details] [diff] [review]
Make MP4Reader::GetBuffered() accurate
Attachment #8450786 - Flags: review?(cpearce)
Created attachment 8450787 [details] [diff] [review]
Fix libstagefright warnings
Attachment #8450787 - Flags: review?(cpearce)
Comment on attachment 8450786 [details] [diff] [review]
Make MP4Reader::GetBuffered() accurate

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

::: content/media/fmp4/MP4Reader.cpp
@@ +563,5 @@
> +  }
> +
> +  nsTArray<Interval<Microseconds>> timeRanges;
> +  {
> +    MonitorAutoLock audioMon(mAudio.mMonitor);

Why do you need to lock the audio and video decoders here? To protect the demuxer? You're not accessing any of the decoder's state, AFAICT. 

Note the caller "pins" the MediaResource, so the underlying cached byte ranges in the media stream can't shrink while we're in this call.

@@ +570,5 @@
> +  }
> +
> +  for (size_t i = 0; i < timeRanges.Length(); i++) {
> +    aBuffered->Add(timeRanges[i].start / 1000000.0,
> +                   timeRanges[i].end / 1000000.0);

Why don't you need to subtract aStartTime from these?

::: content/media/gtest/TestMP4Reader.cpp
@@ +16,5 @@
> +using namespace mozilla::dom;
> +
> +struct TestBinding
> +{
> +  RefPtr<MP4Decoder> decoder;

Please use nsRefPtr instead. We should not be using mozilla::RefPtr any more.

::: media/libstagefright/binding/include/mp4_demuxer/Interval.h
@@ +39,5 @@
> +
> +  T start;
> +  T end;
> +
> +  static void Normalize(const nsTArray<Interval<T> >& aIntervals,

nsTArray<Interval<T>>&  should work in all compilers we support. We use it elsewhere, so you're safe to use it here.

@@ +91,5 @@
> +      }
> +    }
> +  }
> +
> +  static bool IsNormal(const nsTArray<Interval<T> >& aIntervals)

I would call it "IsNormalized", as a "normal" vector means something other than what this is.

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +3652,5 @@
> +
> +bool RangeFinder::Contains(uint64_t start, uint64_t end)
> +{
> +    if (!mRanges.size()) {
> +        return 0;

return false;

@@ +3719,5 @@
> +      }
> +
> +      hasSync |= isSyncSample;
> +      if (!hasSync) {
> +        continue;

inconsistent indentation here.
Attachment #8450787 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #3)
> Why do you need to lock the audio and video decoders here? To protect the
> demuxer? You're not accessing any of the decoder's state, AFAICT. 
> 
> Note the caller "pins" the MediaResource, so the underlying cached byte
> ranges in the media stream can't shrink while we're in this call.

It was failing in SampleIterator::seekTo() in this line:

CHECK(sampleIndex < mStopChunkSampleIndex);

SampleIterator is not thread safe which means we can't call DemuxVideoSample (or DemuxAudioSample)  and ConvertByteRangesToTime concurrently.

> @@ +570,5 @@
> > +  }
> > +
> > +  for (size_t i = 0; i < timeRanges.Length(); i++) {
> > +    aBuffered->Add(timeRanges[i].start / 1000000.0,
> > +                   timeRanges[i].end / 1000000.0);
> 
> Why don't you need to subtract aStartTime from these?

I thought I did that. Not sure what happened to that change.

> > +  static void Normalize(const nsTArray<Interval<T> >& aIntervals,
> 
> nsTArray<Interval<T>>&  should work in all compilers we support. We use it
> elsewhere, so you're safe to use it here.

clang-format was "helping" me here.
Comment on attachment 8450786 [details] [diff] [review]
Make MP4Reader::GetBuffered() accurate

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

OK, remember to subtract aStartTime. r+ with those nits picked.
Attachment #8450786 - Flags: review?(cpearce) → review+
SampleIterator is very stateful so we can't calculate the buffered ranges at the same time as reading audio/video samples. I intend to rewrite it so that we export the sample table after initialising the demuxer and use the copy.
Created attachment 8453562 [details] [diff] [review]
Make MP4Reader::GetBuffered() accurate
Attachment #8453562 - Flags: review?(edwin)
Attachment #8450786 - Attachment is obsolete: true
Created attachment 8453564 [details] [diff] [review]
Unit tests for GetBuffered() and Interval

Carrying r+ for tests that haven't changed
Comment on attachment 8453562 [details] [diff] [review]
Make MP4Reader::GetBuffered() accurate

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

r+ with minor things to fix. I will resist any further bikeshedding over whether `Indice' is a word.

::: media/libstagefright/binding/Index.cpp
@@ +91,5 @@
> +  for (size_t i = 0; i < mIndex.Length(); i++) {
> +    const MediaSource::Indice& indice = mIndex[i];
> +    if (!rangeFinder.Contains(MediaByteRange(indice.start_offset,
> +                                             indice.end_offset))) {
> +      hasSync = false;

IIUC, hasSync should be reset if there is a discontinuity in the index (can this ever happen?). Say if we have ranges like:

index |----|=A=|---|====B====|------|=C=|---|
bytes |---|==A==|-|=====B=====|----|==C==|--|

and "indice" B has a sync frame, then "indice" C will be treated as if it has a sync frame as well.

::: media/libstagefright/binding/include/mp4_demuxer/Interval.h
@@ +43,5 @@
> +
> +  static void Normalize(const nsTArray<Interval<T> >& aIntervals,
> +                        nsTArray<Interval<T> >* aNormalized)
> +  {
> +    MOZ_ASSERT(aNormalized->IsEmpty());

You may want to null check too -- just in case -- or make aNormalized an nsTArray<>&.

@@ +58,5 @@
> +      if (current.Contains(sorted[i])) {
> +        continue;
> +      }
> +      if (current.end >= sorted[i].start) {
> +        current.end = sorted[i].end;

In the strict containment case, you'll want:

current.end = max(current.end, sorted[i].end);

I'm not sure if this case will happen, but in a method called `Normalize' I'd rather expect the worst.

@@ +69,5 @@
> +  }
> +
> +  static void Intersection(const nsTArray<Interval<T> >& a0,
> +                           const nsTArray<Interval<T> >& a1,
> +                           nsTArray<Interval<T> >* aIntersection)

nit: "> >" still here

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +3634,5 @@
> +{
> +  Vector<Indice> index;
> +  for (uint32_t sampleIndex = 0; sampleIndex < mSampleTable->countSamples();
> +       sampleIndex++) {
> +      off64_t offset;

nit: 2- & 4-space indents should be consistent.
Attachment #8453562 - Flags: review?(edwin) → review+
(In reply to Edwin Flores [:eflores] [:edwin] from comment #10)
> current.end = max(current.end, sorted[i].end);

The constructor asserts start <= end but start and end are public. I've added an assertion.
 
> @@ +69,5 @@
> > +  }
> > +
> > +  static void Intersection(const nsTArray<Interval<T> >& a0,
> > +                           const nsTArray<Interval<T> >& a1,
> > +                           nsTArray<Interval<T> >* aIntersection)

Stupid clang-format.
Depends on: 1054809
Created attachment 8506558 [details] [diff] [review]
Unit tests for GetBuffered() and Interval
Attachment #8506558 - Flags: review?(cpearce)
Attachment #8453564 - Attachment is obsolete: true
Attachment #8506558 - Flags: review?(cpearce) → review+
Comment on attachment 8506558 [details] [diff] [review]
Unit tests for GetBuffered() and Interval

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

Whoops, let me actually review this before r+ing it...
Attachment #8506558 - Flags: review+ → review?(cpearce)
Comment on attachment 8506558 [details] [diff] [review]
Unit tests for GetBuffered() and Interval

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

OK, cool.
Attachment #8506558 - Flags: review?(cpearce) → review+
Depends on: 1087492
https://hg.mozilla.org/mozilla-central/rev/e4679c01892b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.