Closed
Bug 1298606
Opened 8 years ago
Closed 8 years ago
MediaSource incorrectly calculating next frame status
Categories
(Core :: Audio/Video: Playback, defect, P1)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: jya, Assigned: jya)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
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•8 years ago
|
Summary: MediaSource incorrectly calculating next fframe status → MediaSource incorrectly calculating next frame status
Updated•8 years ago
|
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•8 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•8 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•8 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•8 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•8 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•8 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
Closed: 8 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.
Description
•