Layout sizes for video elements are too simplistic #31416

jdm · 2024-02-23T06:47:07Z

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/html/semantics/embedded-content/the-video-element/intrinsic_sizes.htm tests various scenarios for sizing video elements. We don't implement the layout rules related to poster elements or aspect ratios at the moment, so we fail them all. The following is a patch that allows the first test (default object size) to pass:

diff --git a/components/layout/display_list/builder.rs b/components/layout/display_list/builder.rs
index 753b7747c9..c42fed5fea 100644
--- a/components/layout/display_list/builder.rs
+++ b/components/layout/display_list/builder.rs
@@ -1879,7 +1879,7 @@ impl Fragment {
                 }
             },
             SpecificFragmentInfo::Media(ref fragment_info) => {
-                if let Some((ref image_key, _, _)) = fragment_info.current_frame {
+                if let Some(ref image_key) = fragment_info.current_frame {
                     let base = create_base_display_item(state);
                     state.add_image_item(
                         base,
diff --git a/components/layout/fragment.rs b/components/layout/fragment.rs
index f22a0edb9a..7cdb81adae 100644
--- a/components/layout/fragment.rs
+++ b/components/layout/fragment.rs
@@ -385,13 +385,17 @@ impl CanvasFragmentInfo {

 #[derive(Clone)]
 pub struct MediaFragmentInfo {
-    pub current_frame: Option<(webrender_api::ImageKey, i32, i32)>,
+    pub current_frame: Option<webrender_api::ImageKey>,
+    pub width: i32,
+    pub height: i32,
 }

 impl MediaFragmentInfo {
     pub fn new(data: HTMLMediaData) -> MediaFragmentInfo {
         MediaFragmentInfo {
             current_frame: data.current_frame,
+            width: data.width,
+            height: data.height,
         }
     }
 }
@@ -1042,11 +1046,7 @@ impl Fragment {
                 }
             },
             SpecificFragmentInfo::Media(ref info) => {
-                if let Some((_, width, _)) = info.current_frame {
-                    Au::from_px(width)
-                } else {
-                    Au(0)
-                }
+                Au::from_px(info.width)
             },
             SpecificFragmentInfo::Canvas(ref info) => info.dom_width,
             SpecificFragmentInfo::Svg(ref info) => info.dom_width,
@@ -1072,11 +1072,7 @@ impl Fragment {
                 }
             },
             SpecificFragmentInfo::Media(ref info) => {
-                if let Some((_, _, height)) = info.current_frame {
-                    Au::from_px(height)
-                } else {
-                    Au(0)
-                }
+                Au::from_px(info.height)
             },
             SpecificFragmentInfo::Canvas(ref info) => info.dom_height,
             SpecificFragmentInfo::Svg(ref info) => info.dom_height,
diff --git a/components/layout_2020/display_list/mod.rs b/components/layout_2020/display_list/mod.rs
index 91fd074608..7e4b9d09d0 100644
--- a/components/layout_2020/display_list/mod.rs
+++ b/components/layout_2020/display_list/mod.rs
@@ -230,14 +230,22 @@ impl Fragment {
                         .translate(containing_block.origin.to_vector());

                     let common = builder.common_properties(rect.to_webrender(), &i.style);
-                    builder.wr().push_image(
-                        &common,
-                        rect.to_webrender(),
-                        image_rendering(i.style.get_inherited_box().image_rendering),
-                        wr::AlphaType::PremultipliedAlpha,
-                        i.image_key,
-                        wr::ColorF::WHITE,
-                    );
+                    if let Some(image_key) = i.image_key {
+                        builder.wr().push_image(
+                            &common,
+                            rect.to_webrender(),
+                            image_rendering(i.style.get_inherited_box().image_rendering),
+                            wr::AlphaType::PremultipliedAlpha,
+                            image_key,
+                            wr::ColorF::WHITE,
+                        );
+                    } else {
+                        builder.wr().push_rect(
+                            &common,
+                            rect.to_webrender(),
+                            wr::ColorF::WHITE,
+                        );
+                    }
                 },
                 Visibility::Hidden => (),
                 Visibility::Collapse => (),
diff --git a/components/layout_2020/dom.rs b/components/layout_2020/dom.rs
index 3843e77f21..f79a9fc91e 100644
--- a/components/layout_2020/dom.rs
+++ b/components/layout_2020/dom.rs
@@ -93,7 +93,7 @@ pub(crate) trait NodeExt<'dom>: 'dom + LayoutNode<'dom> {
     fn as_image(self) -> Option<(Option<Arc<NetImage>>, PhysicalSize<f64>)>;
     fn as_canvas(self) -> Option<(CanvasInfo, PhysicalSize<f64>)>;
     fn as_iframe(self) -> Option<(PipelineId, BrowsingContextId)>;
-    fn as_video(self) -> Option<(webrender_api::ImageKey, PhysicalSize<f64>)>;
+    fn as_video(self) -> Option<(Option<webrender_api::ImageKey>, PhysicalSize<f64>)>;
     fn style(self, context: &LayoutContext) -> ServoArc<ComputedValues>;

     fn get_style_and_layout_data(self) -> Option<StyleAndLayoutData<'dom>>;
@@ -126,11 +126,11 @@ where
         Some((resource, PhysicalSize::new(width, height)))
     }

-    fn as_video(self) -> Option<(webrender_api::ImageKey, PhysicalSize<f64>)> {
+    fn as_video(self) -> Option<(Option<webrender_api::ImageKey>, PhysicalSize<f64>)> {
         let node = self.to_threadsafe();
-        let frame_data = node.media_data()?.current_frame?;
-        let (width, height) = (frame_data.1 as f64, frame_data.2 as f64);
-        Some((frame_data.0, PhysicalSize::new(width, height)))
+        let media_data = node.media_data()?;
+        let (width, height) = (media_data.width as f64, media_data.height as f64);
+        Some((media_data.current_frame, PhysicalSize::new(width, height)))
     }

     fn as_canvas(self) -> Option<(CanvasInfo, PhysicalSize<f64>)> {
diff --git a/components/layout_2020/fragment_tree/fragment.rs b/components/layout_2020/fragment_tree/fragment.rs
index ccbfe229cc..3d88e1fe21 100644
--- a/components/layout_2020/fragment_tree/fragment.rs
+++ b/components/layout_2020/fragment_tree/fragment.rs
@@ -90,7 +90,7 @@ pub(crate) struct ImageFragment {
     pub style: ServoArc<ComputedValues>,
     pub rect: LogicalRect<Length>,
     #[serde(skip_serializing)]
-    pub image_key: ImageKey,
+    pub image_key: Option<ImageKey>,
 }

 #[derive(Serialize)]
diff --git a/components/layout_2020/replaced.rs b/components/layout_2020/replaced.rs
index 2c5276b20b..236bd9c6bb 100644
--- a/components/layout_2020/replaced.rs
+++ b/components/layout_2020/replaced.rs
@@ -127,7 +127,7 @@ pub(crate) enum ReplacedContentKind {
     Image(Option<Arc<Image>>),
     IFrame(IFrameInfo),
     Canvas(CanvasInfo),
-    Video(VideoInfo),
+    Video(Option<VideoInfo>),
 }

 impl ReplacedContent {
@@ -153,7 +153,7 @@ impl ReplacedContent {
                 )
             } else if let Some((image_key, intrinsic_size_in_dots)) = element.as_video() {
                 (
-                    ReplacedContentKind::Video(VideoInfo { image_key }),
+                    ReplacedContentKind::Video(image_key.map(|key| VideoInfo { image_key: key })),
                     Some(intrinsic_size_in_dots),
                 )
             } else {
@@ -269,7 +269,7 @@ impl ReplacedContent {
                             start_corner: LogicalVec2::zero(),
                             size: size.into(),
                         },
-                        image_key,
+                        image_key: Some(image_key),
                     })
                 })
                 .into_iter()
@@ -281,7 +281,7 @@ impl ReplacedContent {
                     start_corner: LogicalVec2::zero(),
                     size: size.into(),
                 },
-                image_key: video.image_key,
+                image_key: video.as_ref().map(|video| video.image_key),
             })],
             ReplacedContentKind::IFrame(iframe) => {
                 vec![Fragment::IFrame(IFrameFragment {
@@ -327,7 +327,7 @@ impl ReplacedContent {
                         start_corner: LogicalVec2::zero(),
                         size: size.into(),
                     },
-                    image_key,
+                    image_key: Some(image_key),
                 })]
             },
         }
diff --git a/components/script/dom/htmlmediaelement.rs b/components/script/dom/htmlmediaelement.rs
index 0b2ed564c1..18b7c7f061 100644
--- a/components/script/dom/htmlmediaelement.rs
+++ b/components/script/dom/htmlmediaelement.rs
@@ -26,7 +26,6 @@ use net_traits::{
     CoreResourceMsg, FetchChannels, FetchMetadata, FetchResponseListener, Metadata, NetworkError,
     ResourceFetchTiming, ResourceTimingType,
 };
-use script_layout_interface::HTMLMediaData;
 use script_traits::{ImageUpdate, WebrenderIpcSender};
 use servo_config::pref;
 use servo_media::player::audio::AudioRenderer;
@@ -66,7 +65,7 @@ use crate::dom::bindings::inheritance::Castable;
 use crate::dom::bindings::num::Finite;
 use crate::dom::bindings::refcounted::Trusted;
 use crate::dom::bindings::reflector::DomObject;
-use crate::dom::bindings::root::{Dom, DomRoot, LayoutDom, MutNullableDom};
+use crate::dom::bindings::root::{Dom, DomRoot, MutNullableDom};
 use crate::dom::bindings::str::{DOMString, USVString};
 use crate::dom::blob::Blob;
 use crate::dom::document::Document;
@@ -1934,6 +1933,10 @@ impl HTMLMediaElement {
         }
     }

+    pub fn get_current_frame_data(&self) -> Option<(ImageKey, i32, i32)> {
+        self.video_renderer.lock().unwrap().current_frame.clone()
+    }
+
     /// By default the audio is rendered through the audio sink automatically
     /// selected by the servo-media Player instance. However, in some cases, like
     /// the WebAudio MediaElementAudioSourceNode, we need to set a custom audio
@@ -2463,20 +2466,6 @@ impl VirtualMethods for HTMLMediaElement {
     }
 }

-pub trait LayoutHTMLMediaElementHelpers {
-    fn data(self) -> HTMLMediaData;
-}
-
-impl LayoutHTMLMediaElementHelpers for LayoutDom<'_, HTMLMediaElement> {
-    #[allow(unsafe_code)]
-    fn data(self) -> HTMLMediaData {
-        let media = unsafe { &*self.unsafe_get() };
-        HTMLMediaData {
-            current_frame: media.video_renderer.lock().unwrap().current_frame.clone(),
-        }
-    }
-}
-
 #[derive(JSTraceable, MallocSizeOf)]
 pub enum MediaElementMicrotask {
     ResourceSelectionTask {
diff --git a/components/script/dom/htmlvideoelement.rs b/components/script/dom/htmlvideoelement.rs
index a9fcd3d87e..2a1d093cd4 100644
--- a/components/script/dom/htmlvideoelement.rs
+++ b/components/script/dom/htmlvideoelement.rs
@@ -20,6 +20,7 @@ use net_traits::{
     CoreResourceMsg, FetchChannels, FetchMetadata, FetchResponseListener, FetchResponseMsg,
     NetworkError, ResourceFetchTiming, ResourceTimingType,
 };
+use script_layout_interface::HTMLMediaData;
 use servo_media::player::video::VideoFrame;
 use servo_url::ServoUrl;

@@ -31,6 +32,7 @@ use crate::dom::bindings::inheritance::Castable;
 use crate::dom::bindings::refcounted::Trusted;
 use crate::dom::bindings::reflector::DomObject;
 use crate::dom::bindings::root::DomRoot;
+use crate::dom::bindings::root::LayoutDom;
 use crate::dom::bindings::str::DOMString;
 use crate::dom::document::Document;
 use crate::dom::element::{AttributeMutation, Element};
@@ -418,3 +420,23 @@ impl PosterFrameFetchContext {
         }
     }
 }
+
+pub trait LayoutHTMLVideoElementHelpers {
+    fn data(self) -> HTMLMediaData;
+}
+
+impl LayoutHTMLVideoElementHelpers for LayoutDom<'_, HTMLVideoElement> {
+    #[allow(unsafe_code)]
+    fn data(self) -> HTMLMediaData {
+        let video = unsafe { &*self.unsafe_get() };
+        let current_frame = video.htmlmediaelement.get_current_frame_data();
+        let current_frame = current_frame.as_ref();
+        let width = video.get_video_width() as i32;
+        let height = video.get_video_height() as i32;
+        HTMLMediaData {
+            current_frame: current_frame.map(|frame| frame.0),
+            width: current_frame.map_or(width, |frame| frame.1),
+            height: current_frame.map_or(height, |frame| frame.2),
+        }
+    }
+}
diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs
index f0d105fa16..4bb7ee99a0 100644
--- a/components/script/dom/node.rs
+++ b/components/script/dom/node.rs
@@ -88,9 +88,9 @@ use crate::dom::htmliframeelement::{HTMLIFrameElement, HTMLIFrameElementLayoutMe
 use crate::dom::htmlimageelement::{HTMLImageElement, LayoutHTMLImageElementHelpers};
 use crate::dom::htmlinputelement::{HTMLInputElement, LayoutHTMLInputElementHelpers};
 use crate::dom::htmllinkelement::HTMLLinkElement;
-use crate::dom::htmlmediaelement::{HTMLMediaElement, LayoutHTMLMediaElementHelpers};
 use crate::dom::htmlstyleelement::HTMLStyleElement;
 use crate::dom::htmltextareaelement::{HTMLTextAreaElement, LayoutHTMLTextAreaElementHelpers};
+use crate::dom::htmlvideoelement::{HTMLVideoElement, LayoutHTMLVideoElementHelpers};
 use crate::dom::mouseevent::MouseEvent;
 use crate::dom::mutationobserver::{Mutation, MutationObserver, RegisteredObserver};
 use crate::dom::nodelist::NodeList;
@@ -1549,7 +1549,7 @@ impl<'dom> LayoutNodeHelpers<'dom> for LayoutDom<'dom, Node> {
     }

     fn media_data(self) -> Option<HTMLMediaData> {
-        self.downcast::<HTMLMediaElement>()
+        self.downcast::<HTMLVideoElement>()
             .map(|media| media.data())
     }

diff --git a/components/shared/script_layout/lib.rs b/components/shared/script_layout/lib.rs
index 0db4222967..993986789b 100644
--- a/components/shared/script_layout/lib.rs
+++ b/components/shared/script_layout/lib.rs
@@ -160,5 +160,7 @@ pub struct PendingImage {
 }

 pub struct HTMLMediaData {
-    pub current_frame: Option<(ImageKey, i32, i32)>,
+    pub current_frame: Option<ImageKey>,
+    pub width: i32,
+    pub height: i32,
 }

jdm · 2024-02-23T06:48:45Z

Relevant specification: https://html.spec.whatwg.org/multipage/media.html#concept-video-intrinsic-width

eerii · 2024-03-10T12:07:20Z

I have been working with this a bit and thanks to your diff and a tad of poking around trying to understand how tags in html get read by the code now I have it passing the first 3 tests (although it is very unrefined as I have to yet think where everything will have to go to be tidy). Is looking into htmlimage a good guide on how things should work? At least in terms of code structuring. Thank you!

jdm · 2024-03-10T12:36:12Z

Yes, I think that's a useful model to follow!

eerii · 2024-03-10T12:37:39Z

Awesome, I'll try to get the final two tests passing and cleaning up everything a bit. Thanks!

eerii · 2024-03-10T15:51:56Z

Huh I think we are failing test 4 (the one when changing the src) because of #31415, since after some manual debugging, the sizes do seem to be changing but the onloadedmetadata event fires when it isn't supposed to so the comparison fails.

The test for reference:

async_test(function(t) {
  var v = document.getElementById("v4");
  var s = getComputedStyle(v);
  v.src = getVideoURI("/media/movie_5") + "?" + new Date() + Math.random();
  v.onerror = t.unreached_func();
  v.onloadedmetadata = t.step_func(function() {
    assert_equals(s.width, '320px');
    assert_equals(s.height, '240px');
    v.removeAttribute("src");
    v.load();
    // Dimensions should be updated only on next layout.
    requestAnimationFrame(t.step_func_done(function() {
      assert_equals(s.width, "300px");
      assert_equals(s.height, "150px");
    }));
  });
}, "default object size after src is removed");

 0:04.60 pid:237591 wh: 300 150
 0:04.64 pid:237591 wh: 300 150
-> It does the first assert here, 300 != 320, so it fails
 0:04.79 pid:237591 wh: 300 150
 0:05.29 pid:237591 wh: 320 240
-> It should do it here
 0:06.15 pid:237591 wh: 320 240
 0:06.18 pid:237591 wh: 320 240

jdm added A-content/media A-layout/2020 https://github.com/servo/servo/wiki/Layout-2020 C-has-patch labels Feb 23, 2024

eerii added a commit to eerii/servo that referenced this issue Mar 7, 2024

feat: patch from @jdm for video layout sizes (servo#31416)

f98f0cf

eerii added a commit to eerii/servo that referenced this issue Mar 9, 2024

feat: patch from @jdm for video layout sizes (servo#31416)

2c30c42

This was referenced Mar 18, 2024

Implement missing ready states transition #31706

Draft

[WIP]: Improve how intrinsic sizes work for videos #31746

Draft

eerii added a commit to eerii/servo that referenced this issue Mar 27, 2024

feat: patch from @jdm for video layout sizes (servo#31416)

89cda4c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Layout sizes for video elements are too simplistic #31416

Layout sizes for video elements are too simplistic #31416

jdm commented Feb 23, 2024

jdm commented Feb 23, 2024

eerii commented Mar 10, 2024

jdm commented Mar 10, 2024

eerii commented Mar 10, 2024

eerii commented Mar 10, 2024

Layout sizes for video elements are too simplistic #31416

Layout sizes for video elements are too simplistic #31416

Comments

jdm commented Feb 23, 2024

jdm commented Feb 23, 2024

eerii commented Mar 10, 2024

jdm commented Mar 10, 2024

eerii commented Mar 10, 2024

eerii commented Mar 10, 2024