Short videos dispatch loadedmetadata before the metadata is present #31415
Assignees
Comments
|
The state machine for this is quite complicated, but specified via https://html.spec.whatwg.org/multipage/media.html#ready-states In some ways it appears our internal state machine is tied to closely to the outer one(available to JS via Requires a good look at the code and spec... |
|
I feel like giving this a try. |
|
Just to be transparent about my progress: I have been and still am studying the spec and code to get a better understanding of the problem. On a general note, my situation (kids, full-time job) only allows for small time windows each day to work on contributions. So my progress is usually slow, but steady. |
|
All right, here is what I understood so far. From https://html.spec.whatwg.org/multipage/media.html#media-data-processing-steps-list:
We set the dimensions and queue a servo/components/script/dom/htmlmediaelement.rs Lines 1704 to 1708 in 4849ba9 We set the ready state to servo/components/script/dom/htmlmediaelement.rs Lines 1704 to 1713 in 4849ba9 which in turn queues a servo/components/script/dom/htmlmediaelement.rs Lines 615 to 620 in 4849ba9 Before this servo/components/script/dom/htmlmediaelement.rs Lines 2798 to 2802 in 4849ba9 This means we need to make sure that we handle that And now I'm lost. How can we achieve that?
@gterzian, I understand your suggestion on a high level, but I don't feel confident enough introducing this into the state machine, yet. One big picture thing I am missing: where do we handle these queued events? Things I skimmed so far: |
When we do something like I think the problem is essentially a mismatch between how we handle the networking messages, like eof, and how we handle the player events. For clarity, the networking stuff is handled through So the problem is the interaction between the handling of the network response eof, and the handling of the servo/components/script/dom/htmlmediaelement.rs Line 1713 in 4849ba9 I assume that simply removing the above will result in other tests failing. So the exact fix, how to resolve the handling of the response eof and the handling of the |
|
Thanks a lot for that detailed response, @gterzian.
This is a very helpful insight! ☝️ I will go back to my cave to process (and most likely come back with more questions). |
|
https://html.spec.whatwg.org/multipage/media.html#concept-media-load-resource is the relevant spec text. In particular: |
|
Further down the page, this step looks interesting: "Once the entire media resource has been fetched (but potentially before any of it has been decoded)" |
|
I've had another look at the spec, and there are two separate things:
servo/components/script/dom/htmlmediaelement.rs Line 2801 in 4849ba9 Above corresponds to "Once the entire media resource has been fetched (but potentially before any of it has been decoded)". This should indeed fire the But, that step should not do anything with the ready state. Instead, this should be done as part of the below servo/components/script/dom/htmlmediaelement.rs Line 1485 in 4849ba9 |
|
Thanks, now things are starting to take shape. How do you find the relevant sections (presumably without much effort) in the spec? |
Good! Let me know if you have more questions or get stuck, I have a pretty clear idea where the changes should be but don't want to spoil the fun for you.
It can be quite hard if the spec changed and/or our implementation does not comform. I find the firing of events to be pretty clear tags. If the spec mentions one, you can find it in Servo by searching for So let's say I rather look for the relevant part in the code, after looking at the spec. If you're lucky there are clear links to the spec in the code and you can search for that... |
Thanks, very much appreciated! 😁
That's a great insight, thanks. |
4 tasks
|
After some intense cross-referencing of the spec on ready states I saw that this state transition was not implemented at all:
So I gave it a try. The test is now failing at video_size_preserved_after_ended.html#26: I guess that's progress! servo/components/script/dom/htmlmediaelement.rs Lines 644 to 647 in da8327a Since some of these methods are just dummy implementations (e.g. is_paused_for_in_band_content() or is_paused_for_user_interaction()), I was not too confident that this would work out of the box in the first place (even though it might be unrelated to the failing assertion).
|
|
Found this in the spec:
Looks like this is not covered yet: I am not sure if this is the cause for the assertion to fail, but it looks like we are not removing a video track, where we should be. 🤔 |
|
Great, sounds like you are on the right track and it just needs some further investigation! |
|
More digging..
I don't know how to translate the highlighted condition to code. servo/components/script/dom/htmlmediaelement.rs Lines 1676 to 1680 in 96e5e77 Then I took a brief detour into servo-media's code, but could not easily relate anything to the spec. I might need to spend more time on that. However, I might not even need that to solve the issue at hand.
I suppose this is probably true for a bunch of places. I marked two of them in dbca037. Next steps: remove video track and fire a |
Sounds about right! I think it's a good idea to go for a specific fix, and leave TODOS and open issues for other stuff you come across. Off-course if a change in one place starts breaking something else, and that you see a fix for that, it's worth doing it in one PR. |
|
I put together a hacky solution passing video_size_preserved_after_ended.html locally to get some feedback via Could many I am wondering how deep I need to go into the avalanche of failing tests caused by my changes. |
|
Clicking into the test runs, it looks like the timeouts might be related to GStreamer not behaving properly on the CI: What happens when you run those tests locally? |
|
Output with
|
If you are able to reproduce the flakiness locally, you should be able to dig in a bit to see why the test sometimes times out. It's likely that there is some timing issue that your change introduces or uncovers. |
|
Just for the record: I only "verified" flakiness with this test: video_size_preserved_after_ended.html, as I was always running it to get feedback on my changes. I haven't tried any other test cases locally up until now. |
|
I ran some of the tests against I haven't tried Full output
// edit // edit 2 |
|
Most of the |
|
I spend the last days studying the spec and code more. Now I think I really understand what this statement means:
I am still trying to fully grasp what parts of the spec should be handled in I will bang my head against the wall for another day or two and if I don't get anywhere I will reach out with questions again. |
|
I would probably start by logging all of the PlayerEvent updates that are received to understand what streamer (via servo-mdedia) is producing. |
Couple of things I'm thinking of:
|
|
Heartbeat: I spent the last couple of days offline with my family; no measurable progress. |
|
Leaving some of my investigative thoughts here:
If we get a I am still wondering if our current player implementation can even signal that it buffered the entire media. I think that would be the signal to set the ready state to If we get a if self.ready_state < ReadyState::HaveMetadata {
self.change_ready_state(ReadyState::HaveNothing);
} else {
self.change_ready_state(ReadyState::HaveMetadata);
} |
In terms of firing the "loadeddata" event, a transition matching
Yes something along those lines seems like a good idea. |
|
track-mode-not-changed-by-new-track.html is really giving me a hard time. Output of
|
|
Is there code that fires load events at track objects yet? |
|
From reading the code, the test that is timing out may actually be making more progress than it used to, so it actually has a chance to time out because we never fire a load events at track objects. |
|
Although that depends on whether the canplaythrough is actually being executed during the test. |
That was one of the things I was looking for while trying to narrow down the origin of the timeout! After frantically jumping around the code I ended up at A quick search for I am not sure what to make of this though. While I don't see any "track" modules listed, there might be some architectural indirection in the code that I am missing. I need to spend way more time in the code base to make an informed judgement.
You have no idea how relieved I am reading this. It did not occur to me that some parts might simply be unimplemented. I was trying to relate my changes to the timout happening at
The canplaythrough callback is being executed. diff --git a/tests/wpt/tests/html/semantics/embedded-content/media-elements/track/track-element/track-mode-not-changed-by-new-track.html b/tests/wpt/tests/html/semantics/embedded-content/media-elements/track/track-element/track-mode-not-changed-by-new-track.html
index 3ec47a39e2..a5f9c6938e 100644
--- a/tests/wpt/tests/html/semantics/embedded-content/media-elements/track/track-element/track-mode-not-changed-by-new-track.html
+++ b/tests/wpt/tests/html/semantics/embedded-content/media-elements/track/track-element/track-mode-not-changed-by-new-track.html
@@ -24,9 +24,11 @@ async_test(function(t) {
assert_equals(track1.track.mode, 'disabled');
assert_equals(track1.track.cues, null);
track1.track.mode = 'hidden';
+ console.log("canplaythrough");
}..and checking the test output: Full output
|
|
Nice! It definitely looks like the rest now fails because of unimplemented functionality rather than buggy functionality. |
|
To be clear: I don't think we should try to bundle the work necessary to make that test pass with your existing changes. It's fine to update the expected result to a timeout. |
I might cry. Then I will go through the failing tests and sort out which of these are timing out at a |
|
Seems like only 2/10 tests are timing out at a I will take a closer look at the others to see why they are timing out: |
|
Heartbeat: I am still investigating why these tests are timing out: I don't have anything concrete enough to drop in a comment yet, but I am amassing investigative notes locally. Why aren't these events fired? I don't know yet. |
|
62964bf fixes the unexpected TIMEOUTs of /css/selectors/media/media-playback-state.html locally. I triggered a WPT run to get the big picture. For the canvas related test cases this nuance in the spec on ready states seems suspiciously relevant:
Intriguing! I am going to give this a closer look in my next free time slot. |
|
Heartbeat: unfortunately I had to put in some extra hours at work, which means no progress on the Servo front. |
|
Heartbeat: I started digging into these two canvas tests: Both of them are stuck on awaiting Looking at the received player events explains why: We never even get to We seem to fetch the entire media resource though: So far I treated the player implementation in servo-media as a black box, which I don't need to change. I tried the far fetched idea of "if the player is buffering, it could be paused on at least one available frame waiting for more data to ensure smooth playback". Setting the ready state to Full output of
|
One thing I am thinking about, and looked a bit into just now, is that perhaps the player is not yet "ready" by the time we start interacting with it from the network listener(like pushing chunks of data). It appears to me that setup needs to be called before any of the signals that send player events can be used. Setup is called only when one of those methods on the player are called: https://github.com/servo/media/blob/45756bef67037ade0f4f0125d579fdc3f3d457c8/backends/gstreamer/player.rs#L789 Back in servo, when we do So the player will only be setup once we start using it, for example at servo/components/script/dom/htmlmediaelement.rs Line 2665 in 4849ba9 So I don't know how the signalling works, see for example https://github.com/servo/media/blob/45756bef67037ade0f4f0125d579fdc3f3d457c8/backends/gstreamer/player.rs#L585 But I wouldn't be surprised is somehow we are missing some player events in some cases because of some interaction between the signal setup in I have no proof for this, but the way of calling The hypothesis could be tested by calling a method on the player, for example |
|
Also see this: https://github.com/servo/media/blob/45756bef67037ade0f4f0125d579fdc3f3d457c8/backends/gstreamer/player.rs#L692 So there is already some synchronization, but perhaps not enough. |
|
@gterzian, thanks a lot for your input. That gave me some more missing pieces of the "how does this player thing work" puzzle. I will start processing and digging most likely on the weekend. |
I tried the following: diff --git a/components/script/dom/htmlmediaelement.rs b/components/script/dom/htmlmediaelement.rs
index 8e1c255b20..797876a4bd 100644
--- a/components/script/dom/htmlmediaelement.rs
+++ b/components/script/dom/htmlmediaelement.rs
@@ -7,7 +7,7 @@ use std::collections::VecDeque;
use std::rc::Rc;
use std::sync::{Arc, Mutex};
use std::time::{Duration, Instant};
-use std::{f64, mem};
+use std::{f64, mem, thread};
use dom_struct::dom_struct;
use embedder_traits::resources::{self, Resource as EmbedderResource};
@@ -917,6 +917,15 @@ impl HTMLMediaElement {
self.queue_dedicated_media_source_failure_steps();
return;
}
+ self.player
+ .borrow()
+ .as_ref()
+ .unwrap()
+ .lock()
+ .unwrap()
+ .set_input_size(256).expect("boom");
+
+ thread::sleep(Duration::from_secs(5));
// Steps 1-2.
// Unapplicable, the `resource` variable already conveys which modeHowever that still results in the same timeout. Full output
I feel like diving into the GStreamer docs again. I am hoping to get a better understanding of how one needs to set up the player and how to interact with it. |
|
Reading the GStreamer did not help me too much. After patching diff --git a/Cargo.toml b/Cargo.toml
index 17f061f8cf..c2c3a9d498 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -91,9 +91,9 @@ selectors = { git = "https://github.com/servo/stylo", branch = "2023-09-01" }
serde = "1.0.197"
serde_bytes = "0.11"
serde_json = "1.0"
-servo-media = { git = "https://github.com/servo/media" }
-servo-media-dummy = { git = "https://github.com/servo/media" }
-servo-media-gstreamer = { git = "https://github.com/servo/media" }
+servo-media = { path = "../media/servo-media" }
+servo-media-dummy = { path = "../media/backends/dummy" }
+servo-media-gstreamer = { path = "../media/backends/gstreamer" }
servo_arc = { git = "https://github.com/servo/stylo", branch = "2023-09-01" }
servo_atoms = { git = "https://github.com/servo/stylo", branch = "2023-09-01" }
size_of_test = { git = "https://github.com/servo/stylo", branch = "2023-09-01" }I added a diff --git a/backends/gstreamer/player.rs b/backends/gstreamer/player.rs
index 8f42906..04374b1 100644
--- a/backends/gstreamer/player.rs
+++ b/backends/gstreamer/player.rs
@@ -1,4 +1,5 @@
use super::BACKEND_BASE_TIME;
+use log::debug;
use crate::media_stream::GStreamerMediaStream;
use crate::media_stream_source::{register_servo_media_stream_src, ServoMediaStreamSrc};
use crate::render::GStreamerRender;
@@ -400,6 +401,7 @@ impl GStreamerPlayer {
}
fn setup(&self) -> Result<(), PlayerError> {
+ debug!("Setting up player");
if self.inner.borrow().is_some() {
return Ok(());
}and ran the test: "Setting up player" was never logged. Full output
Then I thought "Is So I put a diff --git a/backends/gstreamer/player.rs b/backends/gstreamer/player.rs
index 8f42906..c1f7bc9 100644
--- a/backends/gstreamer/player.rs
+++ b/backends/gstreamer/player.rs
@@ -1,4 +1,5 @@
use super::BACKEND_BASE_TIME;
+use log::debug;
use crate::media_stream::GStreamerMediaStream;
use crate::media_stream_source::{register_servo_media_stream_src, ServoMediaStreamSrc};
use crate::render::GStreamerRender;
@@ -379,6 +380,7 @@ impl GStreamerPlayer {
audio_renderer: Option<Arc<Mutex<dyn AudioRenderer>>>,
gl_context: Box<dyn PlayerGLContext>,
) -> GStreamerPlayer {
+ debug!("Creating player with ID: {}", id);
let _ = gst::DebugCategory::new(
"servoplayer",
gst::DebugColorFlags::empty(),..and saw the log: Full output
Does that really confirm that |
|
I think your RUST_LOG may not be set up correctly. The one you need is servo_media_gstreamer=debug to print the log from the gstreamer backend crate. |
|
Oh, wait, the second test shows that that's not an issue. Interesting. Looks like a compelling theory! |
|
Oh wow. I forgot to run servo once before running the test to mitigate this MacOS issue: https://servo.zulipchat.com/#narrow/stream/263398-general/topic/WPT.20Tests.20on.20M1.20Mac/near/429070876 I just tried to run it again and now I see the "Setting up player" logs. Full output
It actually works with both Apologies for the confusion. Now I'm all set to debug the player. |
|
Found another potential lead: We are pushing data into the player. However the player keeps signalling that it needs data by emitting This section from the GstAppSrc docs looks interesting in that regard:
We call servo/components/script/dom/htmlmediaelement.rs Lines 2808 to 2828 in 2904c32 |
|
Neat, 062a0aa passes /html/canvas/element/manual/wide-gamut-canvas/canvas-display-p3-drawImage-video.html locally. // edit: This looks promising! I will go over the unexpected results to see how they relate to 062a0aa. // edit 2: Just to be sure that we have accurate results, I triggered another // edit 3: Two canvas tests are back with unexpected results: When I ran them locally, they sometimes pass and sometimes fail. I will keep digging. |
|
Heartbeat: as we had a major project release at work this week, I was quite busy with bugs-only-users-find-after-release. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is the cause of the failure in the WPT test video_size_preserved_after_ended.html. The video's actual size is obtained when gstreamer sends a player event with the video metadata present (
servo/components/script/dom/htmlmediaelement.rs
Lines 1704 to 1705 in 4849ba9
However, HTMLMediaElement has a special case when the network response EOF is reached and the element's ready state is still HaveNothing (
servo/components/script/dom/htmlmediaelement.rs
Lines 2798 to 2802 in 4849ba9
The text was updated successfully, but these errors were encountered: