From bace54f6d34e8cc112c8d4065ae829fb37ed6b57 Mon Sep 17 00:00:00 2001 From: Matt Wolenetz Date: Mon, 24 May 2021 19:30:44 -0700 Subject: [PATCH] MSE: Fix rare flaky changeType-play-* failures Unless the app explicitly sets `mediaSource.duration`, `HTMLMediaElement.duration` remains NaN until initial HAVE_METADATA is reached, even if the attached mediaSource has already buffered media well beyond the initialization segment(s) necessary to begin transition to HAVE_METADATA. In Chromium, that transition is begun asynchronously, letting thread hop through the pipeline thread complete while letting the app continue. Eventually, the media element transitions, but in the interim, its value for duration could still be NaN, even if mediaSource has a duration value. This change relaxes the changeType-play-* utility's reliance on strict matching of mediaElement.duration and mediaSource.duration, instead relying on the latter for use in trimming the buffered duration. I've filed MSE spec issue #275 to discuss this behavior: https://github.com/w3c/media-source/issues/275 This change also reports more details in changeType web-test assertion failures, which also enabled finding the suspected root cause of the tests' flakiness. Bug: 1184745 Change-Id: I208cbfbbc60776366a16b6a3e79f52480df5be37 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2906776 Auto-Submit: Matthew Wolenetz Reviewed-by: Ted Meyer Commit-Queue: Ted Meyer Cr-Commit-Position: refs/heads/master@{#886148} --- media-source/mediasource-changetype-util.js | 30 +++++++++++++-------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/media-source/mediasource-changetype-util.js b/media-source/mediasource-changetype-util.js index 28e92893657b95..024af027ed69c5 100644 --- a/media-source/mediasource-changetype-util.js +++ b/media-source/mediasource-changetype-util.js @@ -116,10 +116,12 @@ function appendBuffer(test, sourceBuffer, data) { sourceBuffer.appendBuffer(data); } -function trimBuffered(test, mediaElement, sourceBuffer, minimumPreviousDuration, newDuration, skip_duration_prechecks) { +function trimBuffered(test, mediaSource, sourceBuffer, minimumPreviousDuration, newDuration, skip_duration_prechecks) { if (!skip_duration_prechecks) { - assert_less_than(newDuration, minimumPreviousDuration); - assert_less_than(minimumPreviousDuration, mediaElement.duration); + assert_less_than(newDuration, minimumPreviousDuration, + "trimBuffered newDuration must be less than minimumPreviousDuration"); + assert_less_than(minimumPreviousDuration, mediaSource.duration, + "trimBuffered minimumPreviousDuration must be less than mediaSource.duration"); } test.expectEvent(sourceBuffer, "update"); test.expectEvent(sourceBuffer, "updateend"); @@ -128,7 +130,8 @@ function trimBuffered(test, mediaElement, sourceBuffer, minimumPreviousDuration, function trimDuration(test, mediaElement, mediaSource, newDuration, skip_duration_prechecks) { if (!skip_duration_prechecks) { - assert_less_than(newDuration, mediaElement.duration); + assert_less_than(newDuration, mediaSource.duration, + "trimDuration newDuration must be less than mediaSource.duration"); } test.expectEvent(mediaElement, "durationchange"); mediaSource.duration = newDuration; @@ -158,7 +161,8 @@ function runChangeTypeTest(test, mediaElement, mediaSource, metadataA, typeA, da // implicit_changetype and negative_test. function findSafeOffset(targetTime, overlappedMediaMetadata, overlappedStartTime, overlappingMediaMetadata) { - assert_greater_than_equal(targetTime, overlappedStartTime); + assert_greater_than_equal(targetTime, overlappedStartTime, + "findSafeOffset targetTime must be greater than or equal to overlappedStartTime"); let offset = targetTime; if ("start_time" in overlappingMediaMetadata) { @@ -177,7 +181,8 @@ function runChangeTypeTest(test, mediaElement, mediaSource, metadataA, typeA, da let gopsToRetain = Math.ceil((targetTime - overlappedStartTime) / overlappedMediaMetadata["keyframe_interval"]); let adjustedTime = overlappedStartTime + gopsToRetain * overlappedMediaMetadata["keyframe_interval"]; - assert_greater_than_equal(adjustedTime, targetTime); + assert_greater_than_equal(adjustedTime, targetTime, + "findSafeOffset adjustedTime must be greater than or equal to targetTime"); offset += adjustedTime - targetTime; return { "offset": offset, "adjustedTime": adjustedTime }; } @@ -221,7 +226,8 @@ function runChangeTypeTest(test, mediaElement, mediaSource, metadataA, typeA, da // changeType B->B and append B starting at 1.0 seconds (or at the first // keyframe in B at or after 1.0 seconds if it has keyframe_interval defined). test.waitForExpectedEvents(() => { - assert_less_than(lastStart, 1.0); + assert_less_than(lastStart, 1.0, + "changeType B->B lastStart must be less than 1.0"); let safeOffset = findSafeOffset(1.0, metadataB, lastStart, metadataB); lastStart = safeOffset["adjustedTime"]; if (!implicit_changetype) { @@ -239,7 +245,8 @@ function runChangeTypeTest(test, mediaElement, mediaSource, metadataA, typeA, da // changeType B->A and append A starting at 1.5 seconds (or at the first // keyframe in B at or after 1.5 seconds if it has keyframe_interval defined). test.waitForExpectedEvents(() => { - assert_less_than(lastStart, 1.5); + assert_less_than(lastStart, 1.5, + "changeType B->A lastStart must be less than 1.5"); let safeOffset = findSafeOffset(1.5, metadataB, lastStart, metadataA); // Retain the previous lastStart because the next block will append data // which begins between that start time and this block's start time. @@ -258,7 +265,8 @@ function runChangeTypeTest(test, mediaElement, mediaSource, metadataA, typeA, da // changeType A->A and append A starting at 1.3 seconds (or at the first // keyframe in B at or after 1.3 seconds if it has keyframe_interval defined). test.waitForExpectedEvents(() => { - assert_less_than(lastStart, 1.3); + assert_less_than(lastStart, 1.3, + "changeType A->A lastStart must be less than 1.3"); // Our next append will begin by overlapping some of metadataB, then some of // metadataA. let safeOffset = findSafeOffset(1.3, metadataB, lastStart, metadataA); @@ -277,7 +285,7 @@ function runChangeTypeTest(test, mediaElement, mediaSource, metadataA, typeA, da // Trim duration to 2 seconds, then play through to end. test.waitForExpectedEvents(() => { // If negative testing, then skip fragile assertions. - trimBuffered(test, mediaElement, sourceBuffer, 2.1, 2, negative_test); + trimBuffered(test, mediaSource, sourceBuffer, 2.1, 2, negative_test); }); test.waitForExpectedEvents(() => { @@ -286,7 +294,7 @@ function runChangeTypeTest(test, mediaElement, mediaSource, metadataA, typeA, da }); test.waitForExpectedEvents(() => { - assert_equals(mediaElement.currentTime, 0); + assert_equals(mediaElement.currentTime, 0, "currentTime must be 0"); test.expectEvent(mediaSource, "sourceended"); test.expectEvent(mediaElement, "play"); test.expectEvent(mediaElement, "ended");