Media player errors can prevent documents from reaching DocumentReadyState::Complete for a long time

[delan]

To reproduce:

1. Uninstall gstreamer-plugins-good

• Can’t play webm with av1 due to missing gstreamer plugins #31686
• Add gstreamer plugins good/ugly for better codec support #31687

2. RUST_LOG='warn,servoshell<servo@LoadStart,servoshell<servo@HeadParsed,servoshell<servo@LoadComplete,script::dom::document' ./mach run -d 'data:text/html,<video autoplay src="https://video.fosdem.org/2024/h1308/fosdem-2024-2321-embedding-servo-in-rust-projects.av1.webm">'

Output when installed:

[2024-03-15T05:40:39Z TRACE servoshell<servo@LoadStart] TopLevelBrowsingContext(0,1) LoadStart
[2024-03-15T05:40:39Z TRACE servoshell<servo@HeadParsed] TopLevelBrowsingContext(0,1) HeadParsed
[2024-03-15T05:40:41Z DEBUG script::dom::document] Document got finish_load: Media
[2024-03-15T05:40:41Z DEBUG script::dom::document] Document loads are complete.
[components/script/dom/document.rs:1039] state = Complete
[2024-03-15T05:40:41Z DEBUG script::dom::document] About to dispatch load for "data:text/html,<video autoplay src=https... (48b7fac362fd7c61)"
[2024-03-15T05:40:41Z TRACE servoshell<servo@LoadComplete] TopLevelBrowsingContext(0,1) LoadComplete

Output when uninstalled:

[2024-03-15T05:38:05Z TRACE servoshell<servo@LoadStart] TopLevelBrowsingContext(0,1) LoadStart
[2024-03-15T05:38:05Z TRACE servoshell<servo@HeadParsed] TopLevelBrowsingContext(0,1) HeadParsed
[2024-03-15T05:38:07Z ERROR script::dom::htmlmediaelement] Player error: "Error from element /GstPlayBin:playbin/GstURIDecodeBin:uridecodebin0: Your GStreamer installation is missing a plug-in.\nYour GStreamer installation is missing a plug-in.\n../gst/playback/gsturidecodebin.c(1049): no_more_pads_full (): /GstPlayBin:playbin/GstURIDecodeBin:uridecodebin0:\nno suitable plugins found:\n../gst/playback/gstdecodebin2.c(4704): gst_decode_bin_expose (): /GstPlayBin:playbin/GstURIDecodeBin:uridecodebin0/GstDecodeBin:decodebin0:\nno suitable plugins found:\nMissing decoder: WebM (video/webm)\n"

[jdm]

Whether a page is considered complete is affected by the presence of outstanding network fetches for resources, as well as LoadBlocker objects that are attached to the document. For media elements, there is a load blocker defined in
https://github.com/servo/servo/blob/main/components/script/dom/htmlmediaelement.rs#L333 . Since https://github.com/servo/servo/blob/main/components/script/dom/htmlmediaelement.rs#L1002 explicitly disabled load blocking when there's a failure, it would be good to understand what code path triggers an error without running those failure steps.

[frereit]

I'd like to give this a shot and see how far I can get

[frereit]

Hm, so I wrote a one-line patch that fixes this:

3218dd2:

diff --git a/components/script/dom/htmlmediaelement.rs b/components/script/dom/htmlmediaelement.rs
index e9de8cb20d5b..4388b30af7c6 100644
--- a/components/script/dom/htmlmediaelement.rs
+++ b/components/script/dom/htmlmediaelement.rs
@@ -1556,6 +1556,7 @@ impl HTMLMediaElement {
                     MEDIA_ERR_DECODE,
                 )));
                 self.upcast::<EventTarget>().fire_event(atom!("error"));
+                self.queue_dedicated_media_source_failure_steps();
             },
             PlayerEvent::VideoFrameUpdated => {
                 self.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage);

I'd now like to add a test for this but I'm struggling. I think it should be a new unit test but the other unit tests only test very basic functionality of the components. I think I'd have to write a new unit test that initalized a Window for a Document, then adds a HTMLMediaElement to that Document, fire the PlayerEvent::Error event and then validate that delaying_the_load_event_flag is None? This seems like a LOT of boilerplate for what is actually a pretty simple test. Is there a better way to do this or should I go ahead and attempt to implement this test?

[jdm]

One note: the dedicated failure steps already fire an error event (

servo/components/script/dom/htmlmediaelement.rs

Line 1033 in 8cfc6a1

this.upcast::<EventTarget>().fire_event(atom!("error"));

) so we'll need to ensure there isn't any other duplication in this case.

In general, we don't do a lot of unit testing in the script crate. Part of it is for the reasons you identified (lots of boilerplate, and the code structure is written to reflect the specifications), and partly because we usually care about the overall conformance to the web standards instead. Instead, we use integration tests like https://github.com/servo/servo/tree/main/tests/wpt/tests/html/semantics/embedded-content/the-video-element which verify that pages finish loading, fire expected events, display certain content, etc.

https://github.com/servo/servo/blob/main/tests/wpt/tests/html/semantics/embedded-content/media-elements/error-codes/error.html#L23 suggests that there may not be any tests for decoding errors right now, but shows a model for how to write tests that verify the behavior.

[jdm]

See https://github.com/servo/servo/blob/main/tests/wpt/README.md for more information about how to makes use of this test suite.

[frereit]

so we'll need to ensure there isn't any other duplication in this case.

Ah, got it! The same pattern is already present at two other locations, so these are presumably incorrect too. I'll see if I can get some tests running to validate this. Nevermind, I spotted the difference. In these cases the source gets the error event, and this gets it through the queue_dedicated_media_source_failure_steps function.

servo/components/script/dom/htmlmediaelement.rs

Lines 817 to 819 in 8cfc6a1

	source.upcast::<EventTarget>().fire_event(atom!("error"));
	self.queue_dedicated_media_source_failure_steps();
	return;

servo/components/script/dom/htmlmediaelement.rs

Lines 825 to 827 in 8cfc6a1

	source.upcast::<EventTarget>().fire_event(atom!("error"));
	self.queue_dedicated_media_source_failure_steps();
	return;

https://github.com/servo/servo/blob/main/tests/wpt/tests/html/semantics/embedded-content/media-elements/error-codes/error.html#L23 suggests that there may not be any tests for decoding errors right now, but shows a model for how to write tests that verify the behavior.

Thanks! I'll try to create two one tests

• Test if the document finishes loading on player errors
• Test if the error event fires exactly once

thanks for your help

[frereit]

Okay, sorry for all this back and forth / spam, but I actually cannot reproduce the issue as described:

[nix-shell:~/Source/servo]$ git rev-parse HEAD
8cfc6a1898d02eb1df9c13c3c410747cb1e0b412

[nix-shell:~/Source/servo]$ RUST_LOG='warn,servoshell<servo@LoadStart,servoshell<servo@HeadParsed,servoshell<servo@LoadComplete,script::dom::document' ./mach run -d 'data:text/html,<video autoplay src="https://video.fosdem.org/2024/h1308/fosdem-2024-2321-embedding-servo-in-rust-projects.av1.webm">'
[...]
[2024-03-16T22:07:19Z ERROR script::dom::htmlmediaelement] Player error: "Error from element /GstPlayBin:playbin/GstURIDecodeBin:uridecodebin0: Your GStreamer installation is missing a plug-in.\nYour GStreamer installation is missing a plug-in.\n../gst/playback/gsturidecodebin.c(1049): no_more_pads_full (): /GstPlayBin:playbin/GstURIDecodeBin:uridecodebin0:\nno suitable plugins found:\n../gst/playback/gstdecodebin2.c(4704): gst_decode_bin_expose (): /GstPlayBin:playbin/GstURIDecodeBin:uridecodebin0/GstDecodeBin:decodebin0:\nno suitable plugins found:\nMissing decoder: WebM (video/webm)\n"
[2024-03-16T22:07:40Z DEBUG script::dom::document] Document got finish_load: Media
[2024-03-16T22:07:40Z DEBUG script::dom::document] Document loads are complete.
[2024-03-16T22:07:40Z DEBUG script::dom::document] About to dispatch load for "data:text/html,<video autoplay src=\"http... (f8a72f128b37adab)"
[2024-03-16T22:07:40Z TRACE servoshell<servo@LoadComplete] TopLevelBrowsingContext(0,1) LoadComplete

Note how it just takes ~20s for the Document to reach DocumentReadyState::Complete. I'm not sure what causes this but means that the issue would have to be debugged differently because we need to identify where the failure steps are being queued and why they are being queued so late. My guess is that it first downloads the entire video before completing because serving the same file from the local machine causes the document to reach Complete in ~5s on my machine.

@delan: Could you please validate that the document actually never reaches Complete or if it just takes a while? Thank you.

This would also mean that this issue isn't actually a blocker of #31689? I mean sure, it's not great that the spinner would stay on screen longer than neccessary but I don't see how it's a blocker.

[jdm]

Out of curiosity, do we reach the complete state sooner with your changes?

[delan]

Thanks for investigating. Indeed the test case does finish loading for me, though it takes over six minutes!

[2024-03-18T09:49:37Z TRACE servoshell<servo@LoadStart] TopLevelBrowsingContext(0,1) LoadStart
[2024-03-18T09:49:37Z TRACE servoshell<servo@HeadParsed] TopLevelBrowsingContext(0,1) HeadParsed
[2024-03-18T09:49:40Z ERROR script::dom::htmlmediaelement] Player error: "Error from element /GstPlayBin:playbin/GstURIDecodeBin:uridecodebin0: Your GStreamer installation is missing a plug-in.\nYour GStreamer installation is missing a plug-in.\n../gst/playback/gsturidecodebin.c(1049): no_more_pads_full (): /GstPlayBin:playbin/GstURIDecodeBin:uridecodebin0:\nno suitable plugins found:\n../gst/playback/gstdecodebin2.c(4704): gst_decode_bin_expose (): /GstPlayBin:playbin/GstURIDecodeBin:uridecodebin0/GstDecodeBin:decodebin0:\nno suitable plugins found:\nMissing decoder: WebM (video/webm)\n"
[2024-03-18T09:56:13Z DEBUG script::dom::document] Document got finish_load: Media
[2024-03-18T09:56:13Z DEBUG script::dom::document] Document loads are complete.
[2024-03-18T09:56:13Z DEBUG script::dom::document] About to dispatch load for "data:text/html,<video autoplay src=https... (48b7fac362fd7c61)"
[2024-03-18T09:56:13Z TRACE servoshell<servo@LoadComplete] TopLevelBrowsingContext(0,1) LoadComplete

[frereit]

Out of curiosity, do we reach the complete state sooner with your changes?

Yes. With the new changes (#31748) the LoadComplete event fires instantly when a Player Error occurs.

[frereit]

@servo-highfive assign me

[servo-highfive]

Hey @frereit! Thanks for your interest in working on this issue. It's now assigned to you!