Closed
Bug 1298617
Opened 8 years ago
Closed 8 years ago
Search of sample index may return the wrong frame
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)
This problem is made worse with bug 1297036 which increased the fuzz factor. When the trackbuffer sample index is reset, we search the next sample in our array by using the timecode. However, the search is based using a fuzzy search. So we would typically return an earlier frame than the one wanted. This is causing intermittent decoding errors, on mac it shows as some visual corruptions on some mochitests (though it appears on try to cause intermittent failures)
|
||
Updated•8 years ago
|
Priority: -- → P1
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 4•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8785742 [details] Bug 1298617: [MSE] P1. Don't attempt to estimate next sample time if exact value known. https://reviewboard.mozilla.org/r/74826/#review72676 r+ with suggestion: ::: dom/media/mediasource/TrackBuffersManager.cpp:2187 (Diff revision 1) > - trackData.mNextSampleTimecode = > + TimeUnit nextSampleTimecode = > TimeUnit::FromMicroseconds(sample->mTimecode + sample->mDuration); > - trackData.mNextSampleTime = > + TimeUnit nextSampleTime = > TimeUnit::FromMicroseconds(sample->GetEndTime()); Instead of always saving these values here, couldn't you just do the work in the 'else' block below, where they are needed? (Unless 'sample' could be modified in some way? Seems unlikely.)
Attachment #8785742 -
Flags: review?(gsquelart) → review+
|
|
||
Comment 5•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8785743 [details] Bug 1298617: [MSE] P2. Attempt to search the exactly matching sample first. https://reviewboard.mozilla.org/r/74828/#review72678 r+ with potential nit: ::: dom/media/mediasource/TrackBuffersManager.cpp:2254 (Diff revision 1) > MOZ_ASSERT(OnTaskQueue()); > auto& trackData = GetTracksData(aTrack); > const TrackBuffer& track = GetTrackBuffer(aTrack); > > + // Perform an exact search first. > + for (uint32_t i = 0; i < track.Length(); i++) { Isn't there a type mismatch between 'i' (uint32_t) and 'track.Length()' (size_t)?
Attachment #8785743 -
Flags: review?(gsquelart) → review+
|
|
||
Comment 6•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8785744 [details] Bug 1298617: [MSE] P3. Optimize sample search by breaking loop early. https://reviewboard.mozilla.org/r/74830/#review72680
Attachment #8785744 -
Flags: review?(gsquelart) → review+
|
Assignee | |
Comment 7•8 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8785743 [details] Bug 1298617: [MSE] P2. Attempt to search the exactly matching sample first. https://reviewboard.mozilla.org/r/74828/#review72678 > Isn't there a type mismatch between 'i' (uint32_t) and 'track.Length()' (size_t)? so long that both are the same signed it doesn't matter. We can't have more than 100MB of data in an array, so assuming each frame was one byte, uint32_t is still more than plenty
|
|
Assignee | |
Comment 8•8 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8785742 [details] Bug 1298617: [MSE] P1. Don't attempt to estimate next sample time if exact value known. https://reviewboard.mozilla.org/r/74826/#review72676 > Instead of always saving these values here, couldn't you just do the work in the 'else' block below, where they are needed? > (Unless 'sample' could be modified in some way? Seems unlikely.) but they are used ! 2 lignes later as parameter to GetSample However, there's a bug that timecode is passed twice, instead of time.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 15•8 years ago
|
||
Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f57482625bf5 [MSE] P1. Don't attempt to estimate next sample time if exact value known. r=gerald https://hg.mozilla.org/integration/autoland/rev/cd44b608b9f8 [MSE] P2. Attempt to search the exactly matching sample first. r=gerald https://hg.mozilla.org/integration/autoland/rev/6437da20140a [MSE] P3. Optimize sample search by breaking loop early. r=gerald
|
||
Comment 16•8 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/f57482625bf5 https://hg.mozilla.org/mozilla-central/rev/cd44b608b9f8 https://hg.mozilla.org/mozilla-central/rev/6437da20140a
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
•