When constructing a new MediaPlayerPrivate object, the MediaPlayer does not set properties like rate, volume, visible on the private instance, so the instance will use default values, which might be wrong. I'm not sure what the best way to handle them is - add them to the constructor of the private objects or provide a getter on the MediaPlayer objects so the privates can just query them? I'm also not sure what the best way to test this is since the MediaElement doesn't know that the private has wrong values.
I'm just seeing there is getters already, so it looks like this is only an issue with the GStreamer elements.
> I'm just seeing there is getters already, so it looks like this is only an > issue with the GStreamer elements. Agreed, it is the responsibility of the media engine to query the element's state when it is instantiated.
Created attachment 41825 [details] patch
Comment on attachment 41825 [details] patch > LOG_VERBOSE(Media, "Seek: %" GST_TIME_FORMAT, GST_TIME_ARGS(sec)); > - if (!gst_element_seek( m_playBin, m_rate, > + if (!gst_element_seek( m_playBin, m_player->rate(), ^ You could take the oportunity to fix this weird space here =). > - g_object_set(G_OBJECT(m_playBin), "mute", mute, NULL); > + g_object_set(G_OBJECT(m_playBin), "volume", (double) volume, NULL); You should use static_cast<double>() here, mind the space. > @@ -673,12 +657,11 @@ void MediaPlayerPrivate::paint(GraphicsContext* context, const IntRect& rect) > > // paint the rectangle on the context and draw the surface inside. > cairo_translate(cr, rect.x() + gapWidth, rect.y() + gapHeight); > - cairo_rectangle(cr, 0, 0, rect.width(), rect.height()); > cairo_scale(cr, doublePixelAspectRatioNumerator / doublePixelAspectRatioDenominator, > doublePixelAspectRatioDenominator / doublePixelAspectRatioNumerator); > cairo_scale(cr, scale, scale); > cairo_set_source_surface(cr, src, 0, 0); > - cairo_fill(cr); > + cairo_paint(cr); > cairo_restore(cr); These changes look unrelated, can they go in a separate patch? > - void setMuted(bool); Just so it's crystal clear: this is being removed because the mute logic is already handled by HTMLMediaElement? I'll say r- mainly because of the unrelated changes above, but otherwise looks fine to me, so should be good to land after fixing the nits.
Created attachment 41861 [details] patch v2 Here's an updated patch. Sorry about the cairo_paint() - I'll file a separate bug for that. I've had a look and setMuted() is gone from the other MediaPlayerPrivate implementations. The HTMLMediaElement takes care of it these days.
Comment on attachment 41861 [details] patch v2 Thanks!
Comment on attachment 41861 [details] patch v2 Rejecting patch 41861 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11520 test cases. svg/css/getComputedStyle-basic.xhtml -> failed Exiting early after 1 failures. 9845 tests run. 317.97s total testing time 9844 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 6 test cases (<1%) had stderr output
Comment on attachment 41861 [details] patch v2 This failure looks unrelated. Not sure what's going on yet.
It's a real failure, but not your fault. The bots are just way behind. bug 30098.
Comment on attachment 41861 [details] patch v2 Clearing flags on attachment: 41861 Committed r50122: <http://trac.webkit.org/changeset/50122>
All reviewed patches have been landed. Closing bug.