Bug 31155 – [GStreamer] Implement setPreservesPitch()

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

We can change playback rate and still keep the normal pitch with GStreamer ;)

[Sebastian Dröge (slomo)]

Note that this is a non-trivial task and it depends on what exactly the setPreservesPitch() semantics are.

Should it keep the pitch by playing audio at normal speed and skipping every 200ms or by doing complex signal processing to change playback speed of the audio and then adjusting all frequencies?

See http://cgit.freedesktop.org/gstreamer/gstreamer/plain/docs/design/part-trickmodes.txt for some ideas for the first (search for SKIP), that's not implemented yet though.

The second can be done by using the scaletempo or pitch plugins in the audio pipeline... but: they need quite some CPU and the pitch plugin uses a GPL licensed library

[Philippe Normand]

Mac port uses that: http://developer.apple.com/Mac/library/documentation/QuickTime/Reference/QTKitFramework/Classes/QTMovie_Class/Reference/Reference.html#//apple_ref/c/data/QTMovieRateChangesPreservePitchAttribute

"
When the playback rate is not unity, audio must be resampled in order to play at the new rate. The default resampling affects the pitch of the audio (for example, playing at 2x speed raises the pitch by an octave, 1/2x lowers an octave). If this property is set on the movie, an alternative algorithm is used, which alters the speed without changing the pitch. Since this is more computationally expensive, this property may be silently ignored on some slow CPUs.
"

[Philippe Normand]

Some notes:

- scaletempo is soon moving to gst-plugins-good. We should give it a try
- if pitch preserving is enabled set audio-sink to scaletempo ! audioconvert ! audioresample ! autoaudiosink
- create playbin in ::load(), not in player constructor. So we know what to set as audio-sink because ::setPreservesPitch() is called before ::load() in the media element.

[Víctor M. Jáquez L.]

adding me in the CC list

[Víctor M. Jáquez L.]

Ccing Eric Carlson.

Eric, could you shed a light on how to test this feature?

I've a patch but I'm not sure how to test it.

Thanks!

[Eric Carlson]

I have actually been thinking of removing this non-standard attribute. I think preserving pitch during fast play is a useful behavior but it seems that it should be the default behavior on ports that can support it,  and I don't think it is terribly useful for a script to be able to turn it off.

[Víctor M. Jáquez L.]

(In reply to comment #6)
> I have actually been thinking of removing this non-standard attribute. I think preserving pitch during fast play is a useful behavior but it seems that it should be the default behavior on ports that can support it,  and I don't think it is terribly useful for a script to be able to turn it of

for what it's worth, here's the discussion in mozilla on this regard, and it seems they are adopting webkit's approach.

https://bugzilla.mozilla.org/show_bug.cgi?id=495040

[Eric Carlson]

(In reply to comment #7)
> (In reply to comment #6)
> > I have actually been thinking of removing this non-standard attribute. I think preserving pitch during fast play is a useful behavior but it seems that it should be the default behavior on ports that can support it,  and I don't think it is terribly useful for a script to be able to turn it of
> 
> for what it's worth, here's the discussion in mozilla on this regard, and it seems they are adopting webkit's approach.
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=495040

Interesting, I missed that they added mozPreservesPitch. Thanks!

Back to your original question about how to test this: the only thing I can think of is to look at the audio data itself with the WebAudio API.

[Philippe Normand]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > I have actually been thinking of removing this non-standard attribute. I think preserving pitch during fast play is a useful behavior but it seems that it should be the default behavior on ports that can support it,  and I don't think it is terribly useful for a script to be able to turn it of
> > 
> > for what it's worth, here's the discussion in mozilla on this regard, and it seems they are adopting webkit's approach.
> > 
> > https://bugzilla.mozilla.org/show_bug.cgi?id=495040
> 
> Interesting, I missed that they added mozPreservesPitch. Thanks!
> 
> Back to your original question about how to test this: the only thing I can think of is to look at the audio data itself with the WebAudio API.

Good idea! You could use a MediaElementSource node :)
See also bug 78883

[Víctor M. Jáquez L.]

Created attachment 183789 [details]
Patch

[Philippe Normand]

Comment on attachment 183789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183789&action=review

> Source/WebCore/ChangeLog:6
> +        It uses the gstreamer element scaletempo, and creates a new audio-sink

What is It? :) Maybe rephrase a bit a long the lines of "Enable audio pitch preservation by using the scaletempo GStreamer element when required by the MediaPlayer" or something similar

> Source/WebCore/ChangeLog:11
> +        No new tests (OOPS!).

The commit-queue will not like the OOPS here. Can you please instead a layout test shall be implemented at some point using WebAudio APIs as Eric suggested?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1909
> +    // Construct audio sink if pitch preserving is enabled

Missing dot at EOL

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1913
> +    GError* error = 0;

Maybe use a GOwnPtr<GError> here?

[Víctor M. Jáquez L.]

Created attachment 184254 [details]
Patch

[Philippe Normand]

Comment on attachment 184254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184254&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1815
> +    GstElement* convert = gst_element_factory_make("audioconvert", 0);

I think we can safely assume that the elements shipped in gstreamer -core and -plugins-base are available at least. For scaletempo I agree it makes sense to check its presence but the rest, it's a bit overkill, IMHO :)

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1829
> +        GST_WARNING("Failed to create audioresample");

C/P error here

[Víctor M. Jáquez L.]

(In reply to comment #13)
> (From update of attachment 184254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184254&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1815
> > +    GstElement* convert = gst_element_factory_make("audioconvert", 0);
> 
> I think we can safely assume that the elements shipped in gstreamer -core and -plugins-base are available at least. For scaletempo I agree it makes sense to check its presence but the rest, it's a bit overkill, IMHO :)
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1829
> > +        GST_WARNING("Failed to create audioresample");
> 
> C/P error here

Ok. I'll remove the checks for the elements, except the scaletempo one.

[Víctor M. Jáquez L.]

Created attachment 184260 [details]
Patch

[Early Warning System Bot]

Comment on attachment 184260 [details]
Patch

Attachment 184260 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16084257

[Early Warning System Bot]

Comment on attachment 184260 [details]
Patch

Attachment 184260 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16076344

[EFL EWS Bot]

Comment on attachment 184260 [details]
Patch

Attachment 184260 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16075588

[Philippe Normand]

Comment on attachment 184260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184260&action=review

What's going on with the EWS?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1828
> +    GRefPtr<GstPad> pad = gst_element_get_static_pad(scale, "sink");

You need to adopt the pointer

[Víctor M. Jáquez L.]

Created attachment 184489 [details]
Patch

[Philippe Normand]

Comment on attachment 184489 [details]
Patch

Looks good! I'll cq+ once the bubbles turn green

[WebKit Review Bot]

Comment on attachment 184489 [details]
Patch

Clearing flags on attachment: 184489

Committed r140685: <http://trac.webkit.org/changeset/140685>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.