1298606 - MediaSource incorrectly calculating next frame status

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Jean-Yves Avenard [:jya]]

The problem wasn't explicitly introduced with bug 1297580; however it was exacerbated with it.

https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/MediaSourceDecoder.cpp#282

To determine if readyState can change to HAVE_FUTURE_DATA, we check with MSE if we have 250ms ahead of current time.

However, the fuzz factor is set to 500ms; checking that a 250ms interval is contained in another when the fuzz factor is 500ms will always return true.

Additionally, https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/MediaSourceDecoder.cpp#339 ClampInterval algorithm is wrong in using the intersection between two intervals: it just can't work. Bad brain fart there.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1298606: P1. Add Intervals::ContainsStrict method.

Review commit: https://reviewboard.mozilla.org/r/74806/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74806/

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1298606: [MSE] P2. Properly determine next frame status in ended state.

Review commit: https://reviewboard.mozilla.org/r/74808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74808/

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8785733 [details]
Bug 1298606: P1. Add Intervals::ContainsStrict method.

https://reviewboard.mozilla.org/r/74806/#review72646

r+ with style fix while we're here:

::: dom/media/Intervals.h:584
(Diff revision 1)
>        }
>      }
>      return false;
>    }
>  
> +  bool ContainsStrict(const ElemType& aInterval) const {

Style: '{' on new line.
You could take this opportunity to fix the other ones around here (e.g. Contains() above and below), as the majority of this file follows the correct style.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8785734 [details]
Bug 1298606: [MSE] P2. Properly determine next frame status in ended state.

https://reviewboard.mozilla.org/r/74808/#review72648

r+ with style nit:

::: dom/media/mediasource/MediaSourceDecoder.cpp:283
(Diff revision 1)
>    // Use the buffered range to consider if we have the next frame available.
>    TimeUnit currentPosition = TimeUnit::FromMicroseconds(CurrentPosition());
> +  TimeIntervals buffered = GetBuffered();
> +  buffered.SetFuzz(MediaSourceDemuxer::EOS_FUZZ / 2);
>    TimeInterval interval(currentPosition,
> -                        currentPosition + media::TimeUnit::FromMicroseconds(DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED),
> +                        currentPosition + media::TimeUnit::FromMicroseconds(DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED));

Style: This line looks awfully long, could you split it?

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785733 [details]
Bug 1298606: P1. Add Intervals::ContainsStrict method.

https://reviewboard.mozilla.org/r/74806/#review72646

> Style: '{' on new line.
> You could take this opportunity to fix the other ones around here (e.g. Contains() above and below), as the majority of this file follows the correct style.

will add a another patch.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1298606: P3. Fix coding style.

Review commit: https://reviewboard.mozilla.org/r/74892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74892/

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785734 [details]
Bug 1298606: [MSE] P2. Properly determine next frame status in ended state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74808/diff/1-2/

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785733 [details]
Bug 1298606: P1. Add Intervals::ContainsStrict method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74806/diff/1-2/

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785734 [details]
Bug 1298606: [MSE] P2. Properly determine next frame status in ended state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74808/diff/2-3/

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785822 [details]
Bug 1298606: P3. Fix coding style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74892/diff/1-2/

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8785822 [details]
Bug 1298606: P3. Fix coding style.

https://reviewboard.mozilla.org/r/74892/#review72708

r+ with one more to go:

::: dom/media/Intervals.h:595
(Diff revision 2)
>        }
>      }
>      return false;
>    }
>  
>    bool Contains(const T& aX) const {

You missed a newline before '{' here.

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785822 [details]
Bug 1298606: P3. Fix coding style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74892/diff/2-3/

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785822 [details]
Bug 1298606: P3. Fix coding style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74892/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f3e74146cd1
P1. Add Intervals::ContainsStrict method. r=gerald
https://hg.mozilla.org/integration/autoland/rev/5a028fa1fda0
[MSE] P2. Properly determine next frame status in ended state. r=gerald
https://hg.mozilla.org/integration/autoland/rev/7e473a486ce6
P3. Fix coding style. r=gerald

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f3e74146cd1
https://hg.mozilla.org/mozilla-central/rev/5a028fa1fda0
https://hg.mozilla.org/mozilla-central/rev/7e473a486ce6