dom: Abort media element load on decode errors #31748
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Frederik Reiter <hi@frereit.de>
I think these changes do change behavior, though perhaps they are hard to test for other reasons. I wonder if there is a way to write a test for this though. |
mrobinson
added
T-linux-wpt-2013
github-actions
bot
removed
T-linux-wpt-2013
|
🔨 Triggering try run (#8334593698) for Linux WPT |
|
Test results for linux-wpt-layout-2013 from try job (#8334593698): Flaky unexpected result (20)
Stable unexpected results that are known to be intermittent (11)
|
|
Test results for linux-wpt-layout-2020 from try job (#8334593698): Flaky unexpected result (22)
Stable unexpected results that are known to be intermittent (16)
|
|
✨ Try run (#8334593698) succeeded. |
I just spent some time looking into how to test is. Unfortunately it seems pretty difficult to test servo/tests/wpt/tests/media-source/mediasource-errors.html Lines 124 to 131 in d6d903c I tried around with creating an MP4 / webm that triggers a
Edit: I experimented around some more and managed to create a completely empty MP4 file (i.e. valid, but contains no video or audio streams): empty.mp4Reading the file with ffmpeg gives no errors but also shows that it doesn't contain any streams: $ ffprobe -i empty.mp4
Input #0, mov,mp4,m4a,3gp,3g2,mj2, from 'Mpeg4.mp4':
Metadata:
major_brand : isom
minor_version : 512
compatible_brands: isomiso2avc1mp41
Duration: N/A, bitrate: N/ASo this file is a valid MP4. However, both firefox and chromium still only raise a "MEDIA_ERR_SRC_NOT_SUPPORTED". Firefox even says that "Media resource [...] could not be decoded" but does not raise a "MEDIA_ERR_DECODE" (??). Servo also doesn't raise a "MEDIA_ERR_DECODE". As a matter of fact, in Chromium, a MEDIA_ERR_DECODE is only raised during a It seems to me like MEDIA_ERR_DECODE is only raised very exceptional cases, which makes testing this very hard until the Media Source API is available. Would it be acceptable to only add a comment that states that a test can only be written when the Media Source API is available? |
frereit
commented
Mar 19, 2024
Comment on lines
+1575
to
+1576
|
|
||
| // TODO: 6. Abort the overall resource selection algorithm. |
There was a problem hiding this comment.
I believe this is not required here / we can skip this step. There is a similar error path in HTMLMediaElementFetchListener that is also supposed to "Abort the overall resource selection algorithm" and it is skipped there too:
servo/components/script/dom/htmlmediaelement.rs
Lines 2813 to 2833 in 06a021d
Suggested change
| // TODO: 6. Abort the overall resource selection algorithm. |
This makes sense to me and the code change is pretty straight-forward. I've updated the PR description to say this. |
mrobinson
approved these changes
Mar 26, 2024
mrobinson
approved these changes
Mar 26, 2024
ektuu
pushed a commit
to ektuu/servo
that referenced
this pull request
Mar 28, 2024
Signed-off-by: Frederik Reiter <hi@frereit.de>
MunishMummadi
pushed a commit
to MunishMummadi/servo
that referenced
this pull request
Mar 29, 2024
Signed-off-by: Frederik Reiter <hi@frereit.de>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, when a
PlayerEvent::Errorwas encountered, anerrorEvent was fired on the element, but the "delaying-the-load-event" flag was never set to false, so the document was loading until the end of the media source was received inprocess_response_eof:servo/components/script/dom/htmlmediaelement.rs
Line 2811 in d6d903c
I think the issue was that the
PlayerEvent::Errorwas not handled according to spec. I believe the relevant portion is "The media data processing steps list is as follows: ... If the media is corrupted".I've implemented most of the steps but am unsure about "6. Abort the overall resource selection algorithm." Is this even applicable here, or is it "automatically" aborted by cancelling the fetch event?
With this patch, the
LoadCompleteevent is triggered immediately after a media decode error:./mach build -ddoes not report any errors./mach test-tidydoes not report any errors