1129732 - Ensure our start time range is 0

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

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.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Part1. Dynamically adjust calculations using timestampoffset

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.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Patch : Part2. Fix VideoData copy

Includes the mDiscontinuity member when copying VideoData. Those functions were only used by the MDSM before, which doesn't care here about mDiscontinuity.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Fix VideoData copy

Includes the mDiscontinuity member when copying VideoData. Those functions were only used by the MDSM before, which doesn't care here about mDiscontinuity.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Assume buffered range starting close enough to 0 to be 0

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.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Use sourcebufferdecoder's end time

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.

Comment 6

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

Comment on attachment 8560239 [details] [diff] [review]
Part2. Fix VideoData copy

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

Oh yes! Please uplift!

Comment 7

[Matt Woodrow (:mattwoodrow)]

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

Comment 8

[Matt Woodrow (:mattwoodrow)]

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.

Comment 9

[Jean-Yves Avenard [:jya]]

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

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3068d82cb1a8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a190ece892cc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2c7fe98899
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c14bb84abe9

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3068d82cb1a8
https://hg.mozilla.org/mozilla-central/rev/a190ece892cc
https://hg.mozilla.org/mozilla-central/rev/7f2c7fe98899
https://hg.mozilla.org/mozilla-central/rev/8c14bb84abe9

Comment 12

[Ralph Giles (:rillian)]

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.

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 14

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=f24f0b0532f9

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Something in this push caused linux32 debug bustage. Nobody was around to help, so I had to back it out.
https://hg.mozilla.org/releases/mozilla-aurora/rev/77d5e3a30435

https://treeherder.mozilla.org/logviewer.html#?job_id=612948&repo=mozilla-aurora

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Also w-p-t permafail.
https://treeherder.mozilla.org/logviewer.html#?job_id=614652&repo=mozilla-aurora

Comment 17

[Jean-Yves Avenard [:jya]]

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.

Comment 18

[Ralph Giles (:rillian)]

Pushed again after rebasing around bug 1127554.

https://hg.mozilla.org/releases/mozilla-aurora/rev/313cbd7c7c0e
https://hg.mozilla.org/releases/mozilla-aurora/rev/5c82e7305fb0
https://hg.mozilla.org/releases/mozilla-aurora/rev/1c5814ce8ade
https://hg.mozilla.org/releases/mozilla-aurora/rev/6fede35aa9f6

Disabled mp4-a-bitrate test as in 38.

https://hg.mozilla.org/releases/mozilla-aurora/rev/550f936c3293