1192791 - Intervals::operator-= may miss small parts of the interval to remove

Status: RESOLVED FIXED

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

Description

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

Following bug 1191142: With a small eviction threshold (e.g. 9,000,000), youtube videos may gather "holes", eventually leading to a crash in TrackBuffersManager::CheckNextInsertionIndex when seeking and changing resolution repeatedly.

The root cause is actually the '-=' operation on buffered ranges when removing frames, which may leave some buffered range intervals that are not actually present in the frames. CheckNextInsertionIndex relies on a 1:1 correspondance between buffered-ranges and actual frames to work properly.

|i1 -= i2| operates by creating an opposite of i2 and doing an intersection with i1.
Unfortunately when building the complementary of i2, the |+| operator collapses close-enough intervals together according to the fuzz. E.g [4,6 +/-1] becomes [-inf,4 +/-1]+[6 +/-1, +inf], which collapses into [-inf, +inf].

The solution should be to simply ignore the fuzz factor when building the complementary of i2. To be properly vetted and tested!

Comment 1

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

Attachment: 1192791-disregard-fuzz-in-subtraction.patch

When building complementary interval (to intersect with 1st argument), don't use fuzz.
Added unit tests.

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8645684 [details] [diff] [review]
1192791-disregard-fuzz-in-subtraction.patch

Review of attachment 8645684 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/gtest/TestIntervalSet.cpp
@@ +789,5 @@
> +  EXPECT_EQ(10, i0[1].mEnd);
> +
> +  i0 = IntIntervals();
> +  i0 += IntInterval(0, 10);
> +  i2 = IntInterval(4, 6);

probably would be more obvious on what is being test with 4,5 as interval.

@@ +790,5 @@
> +
> +  i0 = IntIntervals();
> +  i0 += IntInterval(0, 10);
> +  i2 = IntInterval(4, 6);
> +  i2.SetFuzz(1);

what about an extra test with setting fuzz on i0?

Comment 3

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

Attachment: 1192791-disregard-fuzz-in-subtraction.patch v2

Better tests as per comment 2. Carrying r+.

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a52be401f2d1

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aab36e44ccf4

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aab36e44ccf4

Comment 7

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

Reminder to uplift to 42, as it fixes potential crashes of MSE code there.

Comment 8

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

Comment on attachment 8645699 [details] [diff] [review]
1192791-disregard-fuzz-in-subtraction.patch v2

Approval Request Comment
[Feature/regressing bug #]: MSE

[User impact if declined]:
When seeking around videos (e.g. YouTube videos), Firefox may crash.
This bug is probably not critical though, as it only happens when heavy eviction happens, probably because the user would have modified the eviction threshold in about:config.

[Describe test coverage new/current, TreeHerder]:
Lots of mochitests covering video playback.
Also this patch adds a unit test for this particular failure.

[Risks and why]: Very low.
The unit test demonstrated the issue before the patch.
And this patch just fixes the way subtracting an interval from an interval set works, but suppressing the 'fuzziness' that would wrongly join nearby intervals, which should just not happen in a subtraction.

[String/UUID change made/needed]: None.

Comment 9

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

Aurora try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c60c5d5f9a2

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8645699 [details] [diff] [review]
1192791-disregard-fuzz-in-subtraction.patch v2

Fix a crash, low risk, taking it!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c138c6fff215