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.
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 :)
(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.
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
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 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 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)...
(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 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 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.
(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.
2009-11-05 05:43 PST, Philippe Normand
2009-11-05 05:45 PST, Philippe Normand
2009-11-05 05:45 PST, Philippe Normand
2009-11-05 05:45 PST, Philippe Normand
2009-11-05 07:02 PST, Philippe Normand
2009-11-05 07:51 PST, Philippe Normand
2009-11-06 00:14 PST, Philippe Normand
2009-11-06 00:14 PST, Philippe Normand
2009-11-06 01:11 PST, Philippe Normand
2009-11-06 01:12 PST, Philippe Normand
2009-11-06 01:13 PST, Philippe Normand
2009-11-06 01:43 PST, Philippe Normand
2009-11-09 01:20 PST, Philippe Normand
2009-11-09 03:20 PST, Philippe Normand
2009-11-10 07:08 PST, Philippe Normand
2009-11-10 07:09 PST, Philippe Normand