Bug 31155 - [GStreamer] Implement setPreservesPitch()
: [GStreamer] Implement setPreservesPitch()
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 103898 108175
:
  Show dependency treegraph
 
Reported: 2009-11-05 01:06 PST by
Modified: 2013-01-29 02:48 PST (History)


Attachments
Patch (3.85 KB, patch)
2013-01-21 08:11 PST, Víctor M. Jáquez L.
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.48 KB, patch)
2013-01-23 09:40 PST, Víctor M. Jáquez L.
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.18 KB, patch)
2013-01-23 10:34 PST, Víctor M. Jáquez L.
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.20 KB, patch)
2013-01-24 07:25 PST, Víctor M. Jáquez L.
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-11-05 01:06:36 PST
We can change playback rate and still keep the normal pitch with GStreamer ;)
------- Comment #1 From 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 From 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 From 2012-11-01 10:24:05 PST -------
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 From 2012-11-12 01:13:26 PST -------
adding me in the CC list
------- Comment #5 From 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 From 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 From 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 From 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 From 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 From 2013-01-21 08:11:56 PST -------
Created an attachment (id=183789) [details]
Patch
------- Comment #11 From 2013-01-21 08:25:54 PST -------
(From update of attachment 183789 [details])
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 From 2013-01-23 09:40:07 PST -------
Created an attachment (id=184254) [details]
Patch
------- Comment #13 From 2013-01-23 09:46:22 PST -------
(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
------- Comment #14 From 2013-01-23 10:03:40 PST -------
(In reply to comment #13)
> (From update of attachment 184254 [details] [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 From 2013-01-23 10:34:13 PST -------
Created an attachment (id=184260) [details]
Patch
------- Comment #16 From 2013-01-23 12:04:51 PST -------
(From update of attachment 184260 [details])
Attachment 184260 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16084257
------- Comment #17 From 2013-01-23 12:36:39 PST -------
(From update of attachment 184260 [details])
Attachment 184260 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16076344
------- Comment #18 From 2013-01-24 00:37:35 PST -------
(From update of attachment 184260 [details])
Attachment 184260 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16075588
------- Comment #19 From 2013-01-24 00:46:56 PST -------
(From update of attachment 184260 [details])
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 From 2013-01-24 07:25:22 PST -------
Created an attachment (id=184489) [details]
Patch
------- Comment #21 From 2013-01-24 07:41:24 PST -------
(From update of attachment 184489 [details])
Looks good! I'll cq+ once the bubbles turn green
------- Comment #22 From 2013-01-24 08:18:11 PST -------
(From update of attachment 184489 [details])
Clearing flags on attachment: 184489

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