Bug 30462 – MediaPlayer doesn't set properties on new privates

• Component: Media
• Status: RESOLVED FIXED

[Benjamin Otte]

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.

[Benjamin Otte]

I'm just seeing there is getters already, so it looks like this is only an issue with the GStreamer elements.

[Eric Carlson]

> 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.

[Benjamin Otte]

Created attachment 41825 [details]
patch

[Gustavo Noronha (kov)]

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.

[Benjamin Otte]

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.

[Gustavo Noronha (kov)]

Comment on attachment 41861 [details]
patch v2

Thanks!

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 41861 [details]
patch v2

This failure looks unrelated.  Not sure what's going on yet.

[Eric Seidel (no email)]

It's a real failure, but not your fault.  The bots are just way behind.  bug 30098.

[WebKit Commit Bot]

Comment on attachment 41861 [details]
patch v2

Clearing flags on attachment: 41861

Committed r50122: <http://trac.webkit.org/changeset/50122>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.