Closed Bug 1192791 Opened 5 years ago Closed 5 years ago

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

Categories

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

1.0 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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!
When building complementary interval (to intersect with 1st argument), don't use fuzz.
Added unit tests.
Assignee: nobody → gsquelart
Attachment #8645684 - Flags: review?(jyavenard)
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?
Attachment #8645684 - Flags: review?(jyavenard) → review+
Better tests as per comment 2. Carrying r+.
Attachment #8645684 - Attachment is obsolete: true
Attachment #8645699 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/aab36e44ccf4
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Reminder to uplift to 42, as it fixes potential crashes of MSE code there.
Flags: needinfo?(gsquelart)
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.
Flags: needinfo?(gsquelart)
Attachment #8645699 - Flags: approval-mozilla-aurora?
Comment on attachment 8645699 [details] [diff] [review]
1192791-disregard-fuzz-in-subtraction.patch v2

Fix a crash, low risk, taking it!
Attachment #8645699 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.