Bug 31047 - [GTK] Failing test media/video-played-ranges-1.html
Summary: [GTK] Failing test media/video-played-ranges-1.html
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-03 01:05 PST by Philippe Normand
Modified: 2009-11-12 02:40 PST (History)
6 users (show)

See Also:


Attachments
fix playback rate setter (6.17 KB, patch)
2009-11-05 05:43 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
don't pause pipeline if already paused, same for play() (2.42 KB, patch)
2009-11-05 05:45 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
unskip fixed tests (1.33 KB, patch)
2009-11-05 05:45 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
webkit coding style fixes (6.53 KB, patch)
2009-11-05 05:45 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
fix playback rate setter (6.38 KB, patch)
2009-11-05 07:02 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
fix playback rate setter (6.90 KB, patch)
2009-11-05 07:51 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
don't pause pipeline if already paused, same for play() (2.42 KB, patch)
2009-11-06 00:14 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
webkit coding style fixes (6.53 KB, patch)
2009-11-06 00:14 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
fix playback rate setter (7.21 KB, patch)
2009-11-06 01:11 PST, Philippe Normand
zecke: review-
Details | Formatted Diff | Diff
don't pause pipeline if already paused, same for play() (2.42 KB, patch)
2009-11-06 01:12 PST, Philippe Normand
zecke: review-
Details | Formatted Diff | Diff
webkit coding style fixes (6.54 KB, patch)
2009-11-06 01:13 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
fix playback rate setter (7.20 KB, patch)
2009-11-06 01:43 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
fix playback rate setter (7.35 KB, patch)
2009-11-09 01:20 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
fix playback rate setter (7.32 KB, patch)
2009-11-09 03:20 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
don't pause pipeline if already paused, same for play() (2.78 KB, patch)
2009-11-10 07:08 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
don't block the UI thread when calling gst_element_get_state() (3.35 KB, patch)
2009-11-10 07:09 PST, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2009-11-03 01:05:07 PST
Failure seems similar to the one fixed in Bug 30589.
Comment 1 Philippe Normand 2009-11-05 05:43:58 PST
Created attachment 42561 [details]
fix playback rate setter
Comment 2 Philippe Normand 2009-11-05 05:45:21 PST
Created attachment 42562 [details]
don't pause pipeline if already paused, same for play()
Comment 3 Philippe Normand 2009-11-05 05:45:26 PST
Created attachment 42563 [details]
unskip fixed tests
Comment 4 Philippe Normand 2009-11-05 05:45:31 PST
Created attachment 42564 [details]
webkit coding style fixes
Comment 5 Philippe Normand 2009-11-05 06:14:11 PST
Quoting the HTML5 spec:

When the playbackRate  is negative (playback is backwards), any corresponding audio must be muted. When the playbackRate is so low or so high that the user agent cannot play audio usefully, the corresponding audio must also be muted. If the playbackRate is not 1.0, the user agent may apply pitch adjustments to the audio as necessary to render it faithfully.

I will update the first patch accordingly.
Comment 6 Philippe Normand 2009-11-05 07:02:38 PST
Created attachment 42569 [details]
fix playback rate setter

Mute audio stream is if rate is negative or too extreme. Pitch control
will be part of another patch as this patch is already big enough :)
Comment 7 Philippe Normand 2009-11-05 07:51:06 PST
Created attachment 42570 [details]
fix playback rate setter
Comment 8 Philippe Normand 2009-11-06 00:14:04 PST
Created attachment 42637 [details]
don't pause pipeline if already paused, same for play()
Comment 9 Philippe Normand 2009-11-06 00:14:29 PST
Created attachment 42638 [details]
webkit coding style fixes
Comment 10 Sebastian Dröge (slomo) 2009-11-06 00:24:53 PST
(In reply to comment #7)
> Created an attachment (id=42570) [details]
> fix playback rate setter

- in playbackPosition(): if returned position is GST_CLOCK_TIME_NONE (i.e. -1) you shouldn't divide it and all that, but handle it different ;) At other places positive or negative infinity is used for this.
- Add a comment why the "<0.8 || > 2" case is there, that's not obvious (nor does it make much sense but the HTML5 spec says so)
- check if the rate in setRate() actually changes, if it doesn't you don't need to do all that complex seeking stuff

Apart from that all patches look good to me.
Comment 11 Philippe Normand 2009-11-06 01:11:07 PST
Created attachment 42639 [details]
fix playback rate setter

- playbackPosition() returns 0 if unknown
- added comments for rate and audio mute
- no need to check old rate in setRate, the HTMLMediaElement already takes care of it before calling MediaPlayer::setRate
Comment 12 Philippe Normand 2009-11-06 01:12:19 PST
Created attachment 42640 [details]
don't pause pipeline if already paused, same for play()

No change, rebased against setRate patch ;)
Comment 13 Philippe Normand 2009-11-06 01:13:16 PST
Created attachment 42641 [details]
webkit coding style fixes

No change, rebased against attachment 42640 [details]
Comment 14 Philippe Normand 2009-11-06 01:43:46 PST
Created attachment 42642 [details]
fix playback rate setter
Comment 15 Eric Seidel (no email) 2009-11-06 11:24:38 PST
My mind is blown by the 4 patches on this bug.  Which of these are actually up for review?  Normally we try to do one-patch-per-bug to avoid such confusion. :)
Comment 16 Eric Seidel (no email) 2009-11-06 11:25:00 PST
Comment on attachment 42563 [details]
unskip fixed tests

Looks OK.
Comment 17 Eric Seidel (no email) 2009-11-06 11:26:17 PST
CCing media folks.
Comment 18 Eric Seidel (no email) 2009-11-06 11:26:55 PST
Comment on attachment 42641 [details]
webkit coding style fixes

Looks OK.
Comment 19 Sebastian Dröge (slomo) 2009-11-06 23:16:55 PST
Last patches look good for me (from the GStreamer point of view)
Comment 20 Holger Freyther 2009-11-07 00:49:32 PST
Comment on attachment 42639 [details]
fix playback rate setter

Could you please improve the ChangeLog? It should be a complete sentence and should describe what and why you do it. E.g. "Fix playback rate setter by remembering the rate was changed ..." I specially wonder what you try to achieve with the gst_element_get_state calls as seeking might or might not take longer than the XYZ * GST_MSECOND...
Comment 21 Holger Freyther 2009-11-07 00:54:56 PST
Comment on attachment 42640 [details]
don't pause pipeline if already paused, same for play()

I'm really allergic to timeouts. First they will block the GUI thread and second they will eventually fail. so increase problems instead of reducing them. If GStreamer really has a problem setting the pipeline to play twice we will have to introduce double states... The one WebKit wants the pipeline to be in and then listening to the state-changed bus message to see how our pipeline is doing... (e.g. we set it to play but the pipeline has a problem and we get an error message)...
Comment 22 Sebastian Dröge (slomo) 2009-11-07 00:57:54 PST
(In reply to comment #21)
> (From update of attachment 42640 [details])
> If GStreamer really has a problem setting the pipeline to play twice

It doesn't... if a state change to playing is currently pending or already finished, setting it to playing again will do nothing (same for all other states).
Comment 23 Philippe Normand 2009-11-09 01:20:04 PST
Created attachment 42739 [details]
fix playback rate setter

updated ChangeLog entry
Comment 24 Jan Alonzo 2009-11-09 03:07:17 PST
Comment on attachment 42739 [details]
fix playback rate setter

Some minor nits.

> +    if (position == GST_CLOCK_TIME_NONE) {
> +        // Position not yet available. Happens if the pipeline is in
> +        // NULL or READY state.
> +        ret = 0;
> +    } else

Can we move the comment before the conditional to avoid the braces?

> +        ret = (float) (position / 1000000000.0);
> +    LOG_VERBOSE(Media, "Position %" GST_TIME_FORMAT, GST_TIME_ARGS(position));
> +

Can you please add a space between ret and LOG_VERBOSE for clarity?
Comment 25 Philippe Normand 2009-11-09 03:20:23 PST
Created attachment 42746 [details]
fix playback rate setter
Comment 26 Jan Alonzo 2009-11-10 01:34:38 PST
Comment on attachment 42746 [details]
fix playback rate setter

r=me. I have a couple of minor suggestions though.

> +    // Position is available only if the pipeline is not in NULL or
> +    // READY state.
> +    if (position != GST_CLOCK_TIME_NONE)
> +        ret = (float) (position / 1000000000.0);

We tend to avoid C-style casts and use static_cast and friends (also exists in a couple of places in this patch).

> +    gst_element_get_state(m_playBin,
> +        &state, &pending, 250 * GST_NSECOND);

And also we tend to not break into multiple lines where a one-liner would suffice.

Other those it's ok.
Comment 27 WebKit Commit Bot 2009-11-10 01:46:00 PST
Comment on attachment 42746 [details]
fix playback rate setter

Clearing flags on attachment: 42746

Committed r50726: <http://trac.webkit.org/changeset/50726>
Comment 28 WebKit Commit Bot 2009-11-10 01:46:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 29 Holger Freyther 2009-11-10 01:49:06 PST
One question. Why do we need to have a non null timeout value?
Comment 30 WebKit Commit Bot 2009-11-10 03:27:28 PST
Comment on attachment 42563 [details]
unskip fixed tests

Clearing flags on attachment: 42563

Committed r50735: <http://trac.webkit.org/changeset/50735>
Comment 31 WebKit Commit Bot 2009-11-10 03:34:55 PST
Comment on attachment 42641 [details]
webkit coding style fixes

Clearing flags on attachment: 42641

Committed r50736: <http://trac.webkit.org/changeset/50736>
Comment 32 WebKit Commit Bot 2009-11-10 03:35:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 33 Philippe Normand 2009-11-10 07:08:25 PST
Created attachment 42861 [details]
don't pause pipeline if already paused, same for play()
Comment 34 Philippe Normand 2009-11-10 07:09:07 PST
Created attachment 42862 [details]
don't block the UI thread when calling gst_element_get_state()
Comment 35 Eric Seidel (no email) 2009-11-10 12:56:19 PST
Should this bug be re-opened?  Generally we try to do one-patch per bug so as to keep the comments from getting too long.
Comment 36 Philippe Normand 2009-11-11 00:41:13 PST
(In reply to comment #35)
> Should this bug be re-opened?  Generally we try to do one-patch per bug so as
> to keep the comments from getting too long.

Yes please reopen. I will avoid sending multiple patches for a single bug in the future.
Comment 37 WebKit Commit Bot 2009-11-12 02:32:32 PST
Comment on attachment 42861 [details]
don't pause pipeline if already paused, same for play()

Clearing flags on attachment: 42861

Committed r50872: <http://trac.webkit.org/changeset/50872>
Comment 38 WebKit Commit Bot 2009-11-12 02:40:03 PST
Comment on attachment 42862 [details]
don't block the UI thread when calling gst_element_get_state()

Clearing flags on attachment: 42862

Committed r50873: <http://trac.webkit.org/changeset/50873>
Comment 39 WebKit Commit Bot 2009-11-12 02:40:10 PST
All reviewed patches have been landed.  Closing bug.