1034444 - Make MP4Reader::GetBuffered() estimation accurate MP4

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Make MP4Reader::GetBuffered() accurate

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix libstagefright warnings

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(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 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://tbpl.mozilla.org/?tree=Try&rev=94d42b96f2b4

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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.

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Make MP4Reader::GetBuffered() accurate

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Unit tests for GetBuffered() and Interval

Carrying r+ for tests that haven't changed

Comment 10

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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.

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(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.

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bd579e7389a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c60cb7a9403

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4bd579e7389a
https://hg.mozilla.org/mozilla-central/rev/7c60cb7a9403

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Unit tests for GetBuffered() and Interval

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

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...

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8506558 [details] [diff] [review]
Unit tests for GetBuffered() and Interval

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

OK, cool.

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://tbpl.mozilla.org/?tree=Try&rev=5a3da0dce2ff

Comment 18

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4679c01892b

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4679c01892b