Bug 30462

Summary: MediaPlayer doesn't set properties on new privates
Product: WebKit Reporter: Benjamin Otte <otte>
Component: MediaAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: commit-queue, eric.carlson, eric
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Description Flags
gustavo: review-
patch v2 none

Description Benjamin Otte 2009-10-16 14:40:13 PDT
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.
Comment 1 Benjamin Otte 2009-10-16 14:42:12 PDT
I'm just seeing there is getters already, so it looks like this is only an issue with the GStreamer elements.
Comment 2 Eric Carlson 2009-10-22 06:31:24 PDT
> 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.
Comment 3 Benjamin Otte 2009-10-25 13:27:36 PDT
Created attachment 41825 [details]
Comment 4 Gustavo Noronha (kov) 2009-10-25 17:36:17 PDT
Comment on attachment 41825 [details]

>      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.
Comment 5 Benjamin Otte 2009-10-26 04:44:08 PDT
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 6 Gustavo Noronha (kov) 2009-10-26 15:37:56 PDT
Comment on attachment 41861 [details]
patch v2

Comment 7 WebKit Commit Bot 2009-10-26 15:58:11 PDT
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 8 Eric Seidel (no email) 2009-10-26 16:19:40 PDT
Comment on attachment 41861 [details]
patch v2

This failure looks unrelated.  Not sure what's going on yet.
Comment 9 Eric Seidel (no email) 2009-10-26 16:23:45 PDT
It's a real failure, but not your fault.  The bots are just way behind.  bug 30098.
Comment 10 WebKit Commit Bot 2009-10-26 19:42:55 PDT
Comment on attachment 41861 [details]
patch v2

Clearing flags on attachment: 41861

Committed r50122: <http://trac.webkit.org/changeset/50122>
Comment 11 WebKit Commit Bot 2009-10-26 19:42:59 PDT
All reviewed patches have been landed.  Closing bug.