The test LayoutTests/media/video-size-intrinsic-scale.html is currently failing. I think it is because the webkit video sink doesn't handle properly the picture aspect ratio data reported by the upstream gstreamer elements. In fact in VideoSinkGstreamer.cpp, the par_n and par_d don't seem to be used.
Turns out to be bugs in qtdemux too: https://bugzilla.gnome.org/show_bug.cgi?id=596326 https://bugzilla.gnome.org/show_bug.cgi?id=596319 And I started working on a patch for our video sink that would properly handle the pixel-aspect-ratio data reported by GStreamer.
One more! ;) https://bugzilla.gnome.org/show_bug.cgi?id=596571
Created attachment 40229 [details] patch to expose p-a-r from the sink to the player, used during natural size calculation
Created attachment 40435 [details] first patch: clean of caps handling in video sink
Created attachment 40436 [details] second patch: handle p-a-r in player and sink
Comment on attachment 40435 [details] first patch: clean of caps handling in video sink > @@ -178,14 +172,13 @@ webkit_video_sink_set_caps(GstBaseSink* bsink, GstCaps* caps) > priv->width = width; > priv->height = height; > > - /* We dont yet use fps or pixel aspect into but handy to have */ > - priv->fps_n = gst_value_get_fraction_numerator(fps); > - priv->fps_d = gst_value_get_fraction_denominator(fps); > + /* We dont yet use fps but handy to have */ > + if (!gst_structure_get_fraction(structure, "framerate", > + &priv->fps_n, &priv->fps_d)) > + return FALSE; So, I fail to see why we would return here, because failing to retrieve something we do not even use failed. Does this failing mean we will, say, fail to play the video? Returning here will only cause the pixel aspect ratio to not be set, in case we would be able to fetch it, but not this, so maybe just set a sane default, like you are doing for pixel aspect ratio.
Comment on attachment 40436 [details] second patch: handle p-a-r in player and sink > if (GstPad* pad = gst_element_get_static_pad(m_videoSink, "sink")) { > - gst_video_get_size(GST_PAD(pad), &x, &y); > + gst_video_get_size(GST_PAD(pad), &width, &height); > + GstCaps* caps = GST_PAD_CAPS(pad); > + gfloat par; > + gint par_n, par_d; I think this is a good oportunity to start fixing the style in this code, regarding variable names. I would recommend turning par_n and par_d into pixelAspectRatioNumerator, and pixelAspectRatioDenominator, and par to pixelAspectRatio. We should go this way at least for MediaPlayerPrivate. VideoSink is less problematic, because I believe we will be moving it out of WebKit sometime, but try to follow it there for now. > + // re-create the cairo surface only if the size changed Comments should be full sentences, with capitalization, and punctuation =). > + if (size != m_size) { > + if (m_surface) > + cairo_surface_destroy(m_surface); > + m_surface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, size.width(), > + size.height()); > + g_object_set(m_videoSink, "surface", m_surface, 0); > + } So this size is after the scaling has been applied in here, right?: > // TODO: We copy the data twice right now. This could be easily improved. > cairo_t* cr = cairo_create(priv->surface); > + cairo_scale(cr, par, 1.0 / par); > cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE); > cairo_set_source_surface(cr, src, 0, 0); > cairo_surface_destroy(src);
(In reply to comment #6) > (From update of attachment 40435 [details]) > > @@ -178,14 +172,13 @@ webkit_video_sink_set_caps(GstBaseSink* bsink, GstCaps* caps) > > priv->width = width; > > priv->height = height; > > > > - /* We dont yet use fps or pixel aspect into but handy to have */ > > - priv->fps_n = gst_value_get_fraction_numerator(fps); > > - priv->fps_d = gst_value_get_fraction_denominator(fps); > > + /* We dont yet use fps but handy to have */ > > + if (!gst_structure_get_fraction(structure, "framerate", > > + &priv->fps_n, &priv->fps_d)) > > + return FALSE; > > So, I fail to see why we would return here, because failing to retrieve > something we do not even use failed. Does this failing mean we will, say, fail > to play the video? Returning here will only cause the pixel aspect ratio to not > be set, in case we would be able to fetch it, but not this, so maybe just set a > sane default, like you are doing for pixel aspect ratio. I only kept the previous behavior of the code in this patch... What would be a "sane default" fps? :)
Created attachment 40497 [details] updated patch with styles fixes
(In reply to comment #6) > (From update of attachment 40435 [details]) > > @@ -178,14 +172,13 @@ webkit_video_sink_set_caps(GstBaseSink* bsink, GstCaps* caps) > > priv->width = width; > > priv->height = height; > > > > - /* We dont yet use fps or pixel aspect into but handy to have */ > > - priv->fps_n = gst_value_get_fraction_numerator(fps); > > - priv->fps_d = gst_value_get_fraction_denominator(fps); > > + /* We dont yet use fps but handy to have */ > > + if (!gst_structure_get_fraction(structure, "framerate", > > + &priv->fps_n, &priv->fps_d)) > > + return FALSE; > > So, I fail to see why we would return here, because failing to retrieve > something we do not even use failed. Does this failing mean we will, say, fail > to play the video? Returning here will only cause the pixel aspect ratio to not > be set, in case we would be able to fetch it, but not this, so maybe just set a > sane default, like you are doing for pixel aspect ratio. As discussed on IRC with slomo :) 09:13:59 < slomo> philn-tp: if there's no framerate something is really broken and you shouldn't play the video 09:14:14 < philn-tp> slomo: ok so the code is fine as it is? 09:14:18 < slomo> philn-tp: even still images have a framerate (0/1) 09:14:21 < slomo> yes
(In reply to comment #10) > As discussed on IRC with slomo :) > > 09:13:59 < slomo> philn-tp: if there's no framerate something is really > broken and you shouldn't play the video > 09:14:14 < philn-tp> slomo: ok so the code is fine as it is? > 09:14:18 < slomo> philn-tp: even still images have a framerate (0/1) > 09:14:21 < slomo> yes Very good, thanks! I recommend using g_return_val_if_fail there, then, since this is really one of those "shouldn't happen" situations.
Created attachment 40501 [details] first patch: clean of caps handling in video sink (updated)
Created attachment 40502 [details] first patch: clean of caps handling in video sink (updated)
Comment on attachment 40497 [details] updated patch with styles fixes Thanks you!
Comment on attachment 40497 [details] updated patch with styles fixes Clearing flags on attachment: 40497 Committed r49019: <http://trac.webkit.org/changeset/49019>
All reviewed patches have been landed. Closing bug.
Reopening so that the other patch can be landed by the script.
Comment on attachment 40502 [details] first patch: clean of caps handling in video sink (updated) Clearing flags on attachment: 40502 Committed r49021: <http://trac.webkit.org/changeset/49021>
Sorry, early closure is tracked by bug 28230. (Assuming there was some way for the commit-queue to know not to close this?)
(In reply to comment #20) > Sorry, early closure is tracked by bug 28230. (Assuming there was some way for > the commit-queue to know not to close this?) Hey, this was caused by the patch not being marked for review, so I don't think it was a problem in the script behavior.