MediaSource incorrectly calculating next frame status

RESOLVED FIXED in Firefox 51

Status

()

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Depends on: 1298594
(Assignee)

Updated

3 years ago
Summary: MediaSource incorrectly calculating next fframe status → MediaSource incorrectly calculating next frame status
Comment hidden (mozreview-request)

Comment 3

3 years ago
mozreview-review
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.
Attachment #8785733 - Flags: review?(gsquelart) → review+

Comment 4

3 years ago
mozreview-review
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?
Attachment #8785734 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 5

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

3 years ago
mozreview-review
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.
Attachment #8785822 - Flags: review?(gsquelart) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

3 years ago
Pushed by jyavenard@mozilla.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2f3e74146cd1
https://hg.mozilla.org/mozilla-central/rev/5a028fa1fda0
https://hg.mozilla.org/mozilla-central/rev/7e473a486ce6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.