Bug 31047 – [GTK] Failing test media/video-played-ranges-1.html

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

Failure seems similar to the one fixed in Bug 30589.

[Philippe Normand]

Created attachment 42561 [details]
fix playback rate setter

[Philippe Normand]

Created attachment 42562 [details]
don't pause pipeline if already paused, same for play()

[Philippe Normand]

Created attachment 42563 [details]
unskip fixed tests

[Philippe Normand]

Created attachment 42564 [details]
webkit coding style fixes

[Philippe Normand]

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.

[Philippe Normand]

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 :)

[Philippe Normand]

Created attachment 42570 [details]
fix playback rate setter

[Philippe Normand]

Created attachment 42637 [details]
don't pause pipeline if already paused, same for play()

[Philippe Normand]

Created attachment 42638 [details]
webkit coding style fixes

[Sebastian Dröge (slomo)]

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

[Philippe Normand]

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

[Philippe Normand]

Created attachment 42640 [details]
don't pause pipeline if already paused, same for play()

No change, rebased against setRate patch ;)

[Philippe Normand]

Created attachment 42641 [details]
webkit coding style fixes

No change, rebased against attachment 42640 [details]

[Philippe Normand]

Created attachment 42642 [details]
fix playback rate setter

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Comment on attachment 42563 [details]
unskip fixed tests

Looks OK.

[Eric Seidel (no email)]

CCing media folks.

[Eric Seidel (no email)]

Comment on attachment 42641 [details]
webkit coding style fixes

Looks OK.

[Sebastian Dröge (slomo)]

Last patches look good for me (from the GStreamer point of view)

[Holger Freyther]

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

[Holger Freyther]

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

[Sebastian Dröge (slomo)]

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

[Philippe Normand]

Created attachment 42739 [details]
fix playback rate setter

updated ChangeLog entry

[Jan Alonzo]

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?

[Philippe Normand]

Created attachment 42746 [details]
fix playback rate setter

[Jan Alonzo]

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.

[WebKit Commit Bot]

Comment on attachment 42746 [details]
fix playback rate setter

Clearing flags on attachment: 42746

Committed r50726: <http://trac.webkit.org/changeset/50726>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Holger Freyther]

One question. Why do we need to have a non null timeout value?

[WebKit Commit Bot]

Comment on attachment 42563 [details]
unskip fixed tests

Clearing flags on attachment: 42563

Committed r50735: <http://trac.webkit.org/changeset/50735>

[WebKit Commit Bot]

Comment on attachment 42641 [details]
webkit coding style fixes

Clearing flags on attachment: 42641

Committed r50736: <http://trac.webkit.org/changeset/50736>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

Created attachment 42861 [details]
don't pause pipeline if already paused, same for play()

[Philippe Normand]

Created attachment 42862 [details]
don't block the UI thread when calling gst_element_get_state()

[Eric Seidel (no email)]

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.

[Philippe Normand]

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

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.