Closed Bug 1298617 Opened 4 years ago Closed 4 years ago

Search of sample index may return the wrong frame

Categories

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

defect

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)
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 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 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+
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
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.
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
You need to log in before you can comment on or make changes to this bug.