Closed Bug 1174981 Opened 5 years ago Closed 5 years ago

Following appendData() or remove() MediaSourceDemuxer may return frame in invalid order

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(2 files, 1 obsolete file)

The MediaSourceDemuxer uses a cache index to know where to fetch the next sample in the track buffer.

This index however is reset whenever the track buffer is modified (either via a call to sourcebuffer.appendBuffer or sourcebuffer.remove()).

It attempts to find where the new location of the next sample is, but it does so using a search on the PTS.

On the next call to GetSample, the next sample in pts order will be returned. This is wrong. it must returns the next sample in dts order.
Assignee: nobody → jyavenard
We move management of the data to the TrackBuffersManager as it contains
precise information on how the samples are being moved.
Attachment #8622915 - Flags: review?(cpearce)
Comment on attachment 8622915 [details] [diff] [review]
Ensure frames are returned in pts order.

This seems to have introduced a regression in youtube's conformance test 46 (SeekTimeUpdate)
Attachment #8622915 - Attachment is obsolete: true
Attachment #8622915 - Flags: review?(cpearce)
We move management of the data to the TrackBuffersManager as it contains
precise information on how the samples are being moved.
Attachment #8623365 - Flags: review?(cpearce)
If the mediasource track demuxer was initialized after appendSegment was completed,
our buffered ranges would be empty.
Attachment #8623366 - Flags: review?(cpearce)
Comment on attachment 8623365 [details] [diff] [review]
P1. Ensure frames are returned in pts order.

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

Passing review to Alfredo. Thanks for taking on this responsibility Alfredo!
Attachment #8623365 - Flags: review?(cpearce) → review?(ayang)
Comment on attachment 8623366 [details] [diff] [review]
P2. Ensure our cached buffered range is setup.

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

Passing review to Alfredo. Thanks for taking on this responsibility Alfredo!
Attachment #8623366 - Flags: review?(cpearce) → review?(ayang)
Attachment #8623365 - Flags: review?(ayang) → review?(gsquelart)
Comment on attachment 8623366 [details] [diff] [review]
P2. Ensure our cached buffered range is setup.

Thank you for help, Gerald.
Attachment #8623366 - Flags: review?(ayang) → review?(gsquelart)
Comment on attachment 8623365 [details] [diff] [review]
P1. Ensure frames are returned in pts order.

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

r+ with typo and lock fixed.
Other nits at your discretion.

::: dom/media/mediasource/MediaSourceDemuxer.cpp
@@ +16,5 @@
>  typedef TrackInfo::TrackType TrackType;
>  using media::TimeUnit;
>  using media::TimeIntervals;
>  
> +// Gap allowed between frames. Due to inaccuracies is determining buffer end

'is' -> 'in'

@@ +344,4 @@
>    }
>    nsRefPtr<SamplesHolder> samples = new SamplesHolder;
>    samples->mSamples.AppendElement(sample);
>    if (mNextRandomAccessPoint.ToMicroseconds() <= sample->mTime) {

Accessing mNextRandomAccessPoint without a lock.

@@ +360,2 @@
>    if (found) {
>      return SkipAccessPointPromise::CreateAndResolve(parsed, __func__);

I think SkipAccessPointPromise takes a uint32_t.
Also, looking at SkipToNextRandomAccessPoint code, 'parsed' can only be positive, so it could be declared as a uint32_t.

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +1619,5 @@
> +TrackBuffersManager::Seek(TrackInfo::TrackType aTrack,
> +                          const TimeUnit& aTime)
> +{
> +  MOZ_ASSERT(OnTaskQueue());
> +  auto& trackBuffer = GetTracksData(aTrack);

Nit: 'GetTracksData' -> 'GetTrackData' (no 's'), because it's only for one track, and to be consistent with GetTrackBuffer.

@@ +1721,5 @@
> +    return p.forget();
> +  }
> +
> +  // Our previous index has been overwritten, attempt to find the new one.
> +  for (uint32_t i = 0; i < track.Length(); i++) {

All these linear searches!
Probably not an issue in most cases, but maybe we should keep an eye on them? I.e.: New bug for later performance investigations.

@@ +1729,5 @@
> +      TimeUnit::FromMicroseconds(sample->mTimecode + sample->mDuration),
> +      aFuzz};
> +
> +    if (sampleInterval.ContainsWithStrictEnd(trackData.mNextSampleTimecode)) {
> +      nsRefPtr<MediaRawData> p = sample->Clone();

Move |nsRefPtr<MediaRawData> p = sample->Clone();| 4 lines lower, just before its first use in |if (!p)|.
It would have made me worry less about its potential pre-null-check use.

@@ +1733,5 @@
> +      nsRefPtr<MediaRawData> p = sample->Clone();
> +      trackData.mNextGetSampleIndex = Some(i+1);
> +      trackData.mNextSampleTimecode = sampleInterval.mEnd;
> +      trackData.mNextSampleTime =
> +        TimeUnit::FromMicroseconds(sample->GetEndTime());

You're updating all these mNextX's even in case Clone() failed, is that really wanted?
Not sure it's important either way, but you may want to document why, to satisfy future readers curiosity.

@@ +1750,5 @@
> +    const nsRefPtr<MediaRawData>& sample = track[i];
> +    TimeInterval sampleInterval{
> +      TimeUnit::FromMicroseconds(sample->mTime),
> +      TimeUnit::FromMicroseconds(sample->GetEndTime()),
> +      aFuzz};

Same loop with just a different sampleInterval. Maybe the common code could be put in a separate method? (But the different outputs may make that awkward, so ok to keep things that way if it's better.)

@@ +1756,5 @@
> +    if (sampleInterval.ContainsWithStrictEnd(trackData.mNextSampleTimecode)) {
> +      nsRefPtr<MediaRawData> p = sample->Clone();
> +      trackData.mNextGetSampleIndex = Some(i+1);
> +      // Estimate decode timestamp of the next sample.
> +      trackData.mNextSampleTimecode = sampleInterval.mEnd;

Same comments as in previous loop.

@@ +1782,5 @@
> +  MOZ_ASSERT(trackData.mNextGetSampleIndex.isSome());
> +  const TrackBuffersManager::TrackBuffer& track = GetTrackBuffer(aTrack);
> +
> +  uint32_t i = trackData.mNextGetSampleIndex.ref();
> +  for (; i < track.Length(); i++) {

Move |uint32_t i = ...| into the |for| statement.

::: dom/media/mediasource/TrackBuffersManager.h
@@ +80,5 @@
>    {
>      return mEnded;
>    }
> +  TimeUnit Seek(TrackInfo::TrackType aTrack, const TimeUnit& aTime);
> +  int32_t SkipToNextRandomAccessPoint(TrackInfo::TrackType aTrack,

What's this int32_t return value? (Not obvious from function name or return type) Please document.
Also, looking at the function code and calling code, it should probably be a uint32_t.
Attachment #8623365 - Flags: review?(gsquelart) → review+
Attachment #8623366 - Flags: review?(gsquelart) → review+
(In reply to Gerald Squelart [:gerald] from comment #8)
> Comment on attachment 8623365 [details] [diff] [review]
> P1. Ensure frames are returned in pts order.
> 
> Review of attachment 8623365 [details] [diff] [review]:
> -----------------------------------------------------------------

> >    nsRefPtr<SamplesHolder> samples = new SamplesHolder;
> >    samples->mSamples.AppendElement(sample);
> >    if (mNextRandomAccessPoint.ToMicroseconds() <= sample->mTime) {
> 
> Accessing mNextRandomAccessPoint without a lock.

We only need to protect mNextRandomAccesspoint with a lock for read in the reader's task queue. mNextRandomAccesspoint is only ever modified on the demuxer's taskqueue.

> 
> @@ +360,2 @@
> >    if (found) {
> >      return SkipAccessPointPromise::CreateAndResolve(parsed, __func__);
> 
> I think SkipAccessPointPromise takes a uint32_t.
> Also, looking at SkipToNextRandomAccessPoint code, 'parsed' can only be
> positive, so it could be declared as a uint32_t.

ok

> Nit: 'GetTracksData' -> 'GetTrackData' (no 's'), because it's only for one
> track, and to be consistent with GetTrackBuffer.

TracksData contain the definition of the tracks of a particular type. It just happen that there's only one track supported at present ; but there could be more.

So we have multiple trackbuffer in TracksData (TracksData keeps an array of TrackBuffer)

> 
> @@ +1721,5 @@
> > +    return p.forget();
> > +  }
> > +
> > +  // Our previous index has been overwritten, attempt to find the new one.
> > +  for (uint32_t i = 0; i < track.Length(); i++) {
> 
> All these linear searches!
> Probably not an issue in most cases, but maybe we should keep an eye on
> them? I.e.: New bug for later performance investigations.

it's there already:
bug 1171760

and don't worry, it's on my list (maybe doing that at Whistler if any of the talks are too boring :) )

> 
> @@ +1729,5 @@
> > +      TimeUnit::FromMicroseconds(sample->mTimecode + sample->mDuration),
> > +      aFuzz};
> > +
> > +    if (sampleInterval.ContainsWithStrictEnd(trackData.mNextSampleTimecode)) {
> > +      nsRefPtr<MediaRawData> p = sample->Clone();
> 
> Move |nsRefPtr<MediaRawData> p = sample->Clone();| 4 lines lower, just
> before its first use in |if (!p)|.
> It would have made me worry less about its potential pre-null-check use.

good catch

> 
> @@ +1733,5 @@
> > +      nsRefPtr<MediaRawData> p = sample->Clone();
> > +      trackData.mNextGetSampleIndex = Some(i+1);
> > +      trackData.mNextSampleTimecode = sampleInterval.mEnd;
> > +      trackData.mNextSampleTime =
> > +        TimeUnit::FromMicroseconds(sample->GetEndTime());
> 
> You're updating all these mNextX's even in case Clone() failed, is that
> really wanted?
> Not sure it's important either way, but you may want to document why, to
> satisfy future readers curiosity.

yeah, error are fatals, so any errors there and we'll never get call back again.

> 
> @@ +1750,5 @@
> > +    const nsRefPtr<MediaRawData>& sample = track[i];
> > +    TimeInterval sampleInterval{
> > +      TimeUnit::FromMicroseconds(sample->mTime),
> > +      TimeUnit::FromMicroseconds(sample->GetEndTime()),
> > +      aFuzz};
> 
> Same loop with just a different sampleInterval. Maybe the common code could
> be put in a separate method? (But the different outputs may make that
> awkward, so ok to keep things that way if it's better.)
> 
> @@ +1756,5 @@
> > +    if (sampleInterval.ContainsWithStrictEnd(trackData.mNextSampleTimecode)) {
> > +      nsRefPtr<MediaRawData> p = sample->Clone();
> > +      trackData.mNextGetSampleIndex = Some(i+1);
> > +      // Estimate decode timestamp of the next sample.
> > +      trackData.mNextSampleTimecode = sampleInterval.mEnd;
> 
> Same comments as in previous loop.
> 
> @@ +1782,5 @@
> > +  MOZ_ASSERT(trackData.mNextGetSampleIndex.isSome());
> > +  const TrackBuffersManager::TrackBuffer& track = GetTrackBuffer(aTrack);
> > +
> > +  uint32_t i = trackData.mNextGetSampleIndex.ref();
> > +  for (; i < track.Length(); i++) {
> 
> Move |uint32_t i = ...| into the |for| statement.
> 
> ::: dom/media/mediasource/TrackBuffersManager.h
> @@ +80,5 @@
> >    {
> >      return mEnded;
> >    }
> > +  TimeUnit Seek(TrackInfo::TrackType aTrack, const TimeUnit& aTime);
> > +  int32_t SkipToNextRandomAccessPoint(TrackInfo::TrackType aTrack,
> 
> What's this int32_t return value? (Not obvious from function name or return
> type) Please document.
> Also, looking at the function code and calling code, it should probably be a
> uint32_t.

remnant of an older implementation where it could have returned a negative value, indicating an error.
https://hg.mozilla.org/mozilla-central/rev/e3004d414d5e
https://hg.mozilla.org/mozilla-central/rev/bab941cb2c3c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1466606
You need to log in before you can comment on or make changes to this bug.