Bug 31155

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

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

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.

adding me in the CC list

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!

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.

(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

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

(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

Created attachment 183789 [details] Patch

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?

Created attachment 184254 [details] Patch

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

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

Created attachment 184260 [details] Patch

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

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

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

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

Created attachment 184489 [details] Patch

Comment on attachment 184489 [details] Patch Looks good! I'll cq+ once the bubbles turn green

Comment on attachment 184489 [details] Patch Clearing flags on attachment: 184489 Committed r140685: <http://trac.webkit.org/changeset/140685>

All reviewed patches have been landed. Closing bug.