Bug 31155

Summary: [GStreamer] Implement setPreservesPitch()
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: WebKit GtkAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, feature-media-reviews, gns, menard, mrobinson, philn, slomo, vjaquez, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Mac OS X 10.5   
Bug Depends on: 103898, 108175    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Philippe Normand 2009-11-05 01:06:36 PST
We can change playback rate and still keep the normal pitch with GStreamer ;)
Comment 1 Sebastian Dröge (slomo) 2009-11-05 01:18:28 PST
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
Comment 2 Philippe Normand 2010-02-24 06:57:13 PST
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.
"
Comment 3 Philippe Normand 2012-11-01 10:24:05 PDT
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.
Comment 4 Víctor M. Jáquez L. 2012-11-12 01:13:26 PST
adding me in the CC list
Comment 5 Víctor M. Jáquez L. 2013-01-15 03:01:58 PST
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!
Comment 6 Eric Carlson 2013-01-15 06:56:39 PST
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.
Comment 7 Víctor M. Jáquez L. 2013-01-15 08:25:07 PST
(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
Comment 8 Eric Carlson 2013-01-15 08:52:41 PST
(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.
Comment 9 Philippe Normand 2013-01-15 08:54:49 PST
(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
Comment 10 Víctor M. Jáquez L. 2013-01-21 08:11:56 PST
Created attachment 183789 [details]
Patch
Comment 11 Philippe Normand 2013-01-21 08:25:54 PST
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?
Comment 12 Víctor M. Jáquez L. 2013-01-23 09:40:07 PST
Created attachment 184254 [details]
Patch
Comment 13 Philippe Normand 2013-01-23 09:46:22 PST
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
Comment 14 Víctor M. Jáquez L. 2013-01-23 10:03:40 PST
(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.
Comment 15 Víctor M. Jáquez L. 2013-01-23 10:34:13 PST
Created attachment 184260 [details]
Patch
Comment 16 Early Warning System Bot 2013-01-23 12:04:51 PST
Comment on attachment 184260 [details]
Patch

Attachment 184260 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16084257
Comment 17 Early Warning System Bot 2013-01-23 12:36:39 PST
Comment on attachment 184260 [details]
Patch

Attachment 184260 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16076344
Comment 18 EFL EWS Bot 2013-01-24 00:37:35 PST
Comment on attachment 184260 [details]
Patch

Attachment 184260 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16075588
Comment 19 Philippe Normand 2013-01-24 00:46:56 PST
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
Comment 20 Víctor M. Jáquez L. 2013-01-24 07:25:22 PST
Created attachment 184489 [details]
Patch
Comment 21 Philippe Normand 2013-01-24 07:41:24 PST
Comment on attachment 184489 [details]
Patch

Looks good! I'll cq+ once the bubbles turn green
Comment 22 WebKit Review Bot 2013-01-24 08:18:11 PST
Comment on attachment 184489 [details]
Patch

Clearing flags on attachment: 184489

Committed r140685: <http://trac.webkit.org/changeset/140685>
Comment 23 WebKit Review Bot 2013-01-24 08:18:16 PST
All reviewed patches have been landed.  Closing bug.