Ensure our start time range is 0

RESOLVED FIXED in Firefox 37

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks 1 bug)

Trunk
mozilla38
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 unaffected, firefox37 fixed, firefox38 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Assignee

Description

4 years ago
http://w3c.github.io/media-source/#presentation-start-time
"The presentation start time is the earliest time point in the presentation and specifies the initial playback position and earliest possible position. All presentations created using this specification have a presentation start time of 0."

And we currently enforce that the start time provided to the Media Decoder State Machine is exactly 0 (bug 1089480)

However, we do nothing in regards to the data being added, which due to MP4 encoded structure may not always be exactly 0.

So what happens is that we attempt to play media at position 0, which may or may not exist.
Currently the MediaSourceReadBuffer does a bit of a hack in that the first time it request audio or video data, it takes the first sample available and continue from there. But this will cause all our seeks to be slightly off in those media.

It also causes most of the remaining YouTube test to fail as it checks that the buffered start is exactly 0.
Same thing with the web-ref tests.
Assignee

Updated

4 years ago
Blocks: 1130237
Assignee

Comment 1

4 years ago
Make timestampOffset value dynamic and calculated at decode time. MediaSource related objects are the only ones now dealing with buffered range and time values adjusted according to the timestampOffset. The underlying decoders/readers only use time value in their own reference. The completely abstract the time range offset and allows to vary it on the fly.
Attachment #8560237 - Flags: review?(matt.woodrow)
Assignee

Comment 2

4 years ago
Includes the mDiscontinuity member when copying VideoData. Those functions were only used by the MDSM before, which doesn't care here about mDiscontinuity.
Attachment #8560238 - Flags: review?(cpearce)
Assignee

Comment 3

4 years ago
Includes the mDiscontinuity member when copying VideoData. Those functions were only used by the MDSM before, which doesn't care here about mDiscontinuity.
Attachment #8560239 - Flags: review?(cpearce)
Assignee

Updated

4 years ago
Attachment #8560238 - Attachment is obsolete: true
Attachment #8560238 - Flags: review?(cpearce)
Assignee

Comment 4

4 years ago
Massage the buffered range so if their start is close enough to 0, we make them 0. This is done by slightly adjusting the internal timestamp offset. We assume that the entire source buffer is affected by the same offset. This is almost identical to what Chrome is doing. IE has a slightly different behaviour (wrong IMHO) in that they only adjust the start of a buffered range. The end times stay the same ; which cause the various duration to be extended.
Chrome on the other hand, while it adjust all the buffered ranges to start slightly earlier, set the mediasource.duration with the non-adjusted end value. Which I believe is wrong too as it's not consistent and you happen to have a mediasource duration slightly longer than the source buffers' time range.
Attachment #8560240 - Flags: review?(matt.woodrow)
Assignee

Comment 5

4 years ago
We must use the SourceBufferDecoder end time rather than the sub-reader's one ; as if the source buffer resource had been trimmed, the value would have been incorrect.
Attachment #8560277 - Flags: review?(matt.woodrow)
Comment on attachment 8560239 [details] [diff] [review]
Part2. Fix VideoData copy

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

Oh yes! Please uplift!
Attachment #8560239 - Flags: review?(cpearce) → review+
Comment on attachment 8560237 [details] [diff] [review]
Part1. Dynamically adjust calculations using timestampoffset

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

::: dom/html/TimeRanges.h
@@ +60,5 @@
>    virtual double Start(uint32_t aIndex, ErrorResult& aRv);
>  
>    virtual double End(uint32_t aIndex, ErrorResult& aRv);
>  
> +  virtual void Shift(double aShift);

This shouldn't be virtual.

::: dom/media/MediaData.h
@@ +88,5 @@
> +  // specified timestamp and duration. All data from aOther is copied
> +  // into the new AudioData but the audio data which is transferred.
> +  // After such call, the original aOther is unusable.
> +  static already_AddRefed<AudioData>
> +  TransferWithUpdateTimestampAndDuration(AudioData* aOther,

TransferAndUpdate, or TransferWithUpdated

::: dom/media/mediasource/MediaSourceReader.cpp
@@ +463,3 @@
>  
>      nsRefPtr<dom::TimeRanges> ranges = new dom::TimeRanges();
>      aTrackDecoders[i]->GetBuffered(ranges);

use newDecoder here?

::: media/libstagefright/binding/MoofParser.cpp
@@ +213,5 @@
>      }
>    }
>  }
>  
> +Moof::Moof(Box& aBox, Trex& aTrex, Mdhd& aMdhd, Edts& aEdts, Sinf& aSinf) :

Put the : on the second line
Attachment #8560237 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8560237 [details] [diff] [review]
Part1. Dynamically adjust calculations using timestampoffset

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

Fwiw this would have been *much* easier to review if removing the old timestamp offset code, and converting MSR to use SourceBufferDecoder had been split into separate patches.
Attachment #8560277 - Flags: review?(matt.woodrow) → review+
Attachment #8560240 - Flags: review?(matt.woodrow) → review-
Attachment #8560240 - Flags: review- → review+
Assignee

Comment 9

4 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> Comment on attachment 8560237 [details] [diff] [review]
> Part1. Dynamically adjust calculations using timestampoffset
> 
> Review of attachment 8560237 [details] [diff] [review]:
> -----------------------------------------------------------------
> > +  TransferWithUpdateTimestampAndDuration(AudioData* aOther,
> 
> TransferAndUpdate, or TransferWithUpdated

Existing similar functions were:
ShallowCopyUpdateDuration
ShallowCopyUpdateTimestamp
and:
ShallowCopyUpdateTimestampAndDuration
Comment on attachment 8560237 [details] [diff] [review]
Part1. Dynamically adjust calculations using timestampoffset

Requesting 37 uplift of all patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Spec compliance, possible stalls with youtube video. Less consistent testing and harder to port critical fixes.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This affects non-MSE playback, but has had time to settle on nightly. Risk should be low.
[String/UUID change made/needed]: None.
Attachment #8560237 - Flags: approval-mozilla-aurora?
Comment on attachment 8560237 [details] [diff] [review]
Part1. Dynamically adjust calculations using timestampoffset

This is a rather large patch but a prerep for further MSE work. It has also been on m-c for a week with no noticeable fallout yet. Let's get this onto Aurora for the weekend and see if anything shakes out before this merges to Beta on Monday. Aurora+
Attachment #8560237 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8560239 - Flags: approval-mozilla-aurora+
Attachment #8560240 - Flags: approval-mozilla-aurora+
Attachment #8560277 - Flags: approval-mozilla-aurora+
Assignee

Comment 17

4 years ago
The MP4/webm bitrate tests were disabled in central. They test a feature we haven't implemented. The new result you are seeing is actually the result it should return.
You need to log in before you can comment on or make changes to this bug.