Audio decoder #52

ferjm · 2018-06-13T00:57:38Z

Fixes #41


        Basic audio decoder. Using audio sink to test for now

sdroege · 2018-06-13T05:50:42Z

servo-media/src/backends/gstreamer/audio_decoder.rs

+                    .map_err(|_| ())
+            };
+
+            if !is_audio || insert_sink().is_err() {


You probably also do not want to add a new sink for each audio stream (there could be multiple), but only take one. Or do you want to take them all?

sdroege · 2018-06-13T05:51:49Z

servo-media/src/backends/gstreamer/audio_decoder.rs

+        });
+
+        let appsrc = appsrc.downcast::<AppSrc>().map_err(|_| ())?;
+        appsrc.set_property_format(gst::Format::Time);


Time format requires you to properly timestamp the data you put into the appsrc for each buffer. You probably can't do that, so set it to bytes instead :)

sdroege · 2018-06-13T05:52:29Z

servo-media/src/backends/gstreamer/audio_decoder.rs

+                    appsrc_
+                        .push_buffer(buffer)
+                        .into_result()
+                        .expect("pushing buffer failed");


This should be cleanly handled instead of just panicking

sdroege · 2018-06-13T05:53:40Z

servo-media/src/backends/gstreamer/audio_decoder.rs

+                        buffer
+                            .copy_from_slice(0, &data[offset..next_offset])
+                            .expect("copying failed");
+                        offset = next_offset;


You could use std::io::Cursor and Read on the Vec to prevent having to keep track of the offset yourself


        Address review feedback: use Cursor, control sink addition and others


        AppSink. Test with channel based API. Still WIP. Lots of buffer copying


        AudioDecoderCallbacks


        Avoid extra buffer copy


        Remove unused AudioDecoderMsg

ferjm · 2018-06-18T15:52:20Z

With these patches I am able to decode and play an .mp3 file, but with very poor quality (lots of noise). I am not sure why yet.

philn · 2018-06-18T16:35:57Z

Seems like you need a capsfilter before the appsink so that the audio samples are converted to the right format expected by the backend.

sdroege · 2018-06-18T16:49:13Z

Seems like you need a capsfilter before the appsink so that the audio samples are converted to the right format expected by the backend.

Or you set the caps property on appsink accordingly. That has the same effect and is fewer elements

sdroege · 2018-06-18T16:49:35Z

examples/audio_decoder.rs

+        )),
+    );
+    graph.resume();
+    thread::sleep(time::Duration::from_millis(5000));


Because if the graph is dropped, the rendering thread is closed and we won't hear the audio playback.


        Set AppSink caps

ferjm · 2018-06-18T17:43:05Z

With the appsink caps set, the sound is perfect. Thank you!

Manishearth

Looks okay. I don't understand gstreamer enough to comment much on the actual decode implementation but Sebastian and Philippe seem to be helping with that 😄

Manishearth · 2018-06-18T20:50:32Z

servo-media/src/audio/graph.rs

+
+    /// Asynchronously decodes the audio file data contained in the given
+    /// buffer.
+    pub fn decode_audio_data(&self, data: Vec<u8>, callbacks: AudioDecoderCallbacks) {


We should eventually split out the GStreamer stuff as a separate crate with traits, but that's going to be annoying for now so we don't need to do it yet 😄

(it's also low priority)

We already have the AudioSink and AudioDecoder traits, so it shouldn't be too hard ™ :)

Manishearth · 2018-06-18T20:50:32Z

examples/audio_decoder.rs

+                .extend_from_slice((*buffer).as_ref());
+        })
+        .build();
+    graph.decode_audio_data(bytes.to_vec(), callbacks);


Should we eventually make it such that this can be plugged into a source element on the render thread side, or do we want this to be separate to make the network stuff work?

For media elements (and MediaElementAudioSourceNodes) we will likely use a different approach and use GstPlayer directly (which handles all the decoding work internally), check #43.

For MediaStream*AudioSourceNodes, I am not so sure yet, I still need to read more of these specs, but in general, I have the feeling that all decoding should happen outside of any WebAudio source element, and certainly off the render thread.

In any case, AudioDecoder and GStreamerAudioDecoder are meant to be reusable.

and certainly off the render thread.

Note that this is trivial with GStreamer, so shout when you need to move things to different threads, etc and I'll help :)

Manishearth · 2018-06-18T20:50:32Z

examples/audio_decoder.rs

+    println!("Decoding audio");
+    receiver.recv().unwrap();
+    println!("Audio decoded");
+    let buffer_source = graph.create_node(AudioNodeType::AudioBufferSourceNode);


How do we plan to make this work with ongoing network fetches? Perhaps a BufferSourceNode you can message extra buffers to?

(not something we need to solve now, just want your thoughts)

AFAICT AudioBufferSourceNodes are meant for in-memory audio assets. For network-backed assets the spec suggests the usage of AudioWorkletNodes, which is a completely different war :).

Manishearth · 2018-06-18T20:50:32Z

servo-media/src/backends/gstreamer/audio_decoder.rs

+                let pipeline_ = pipeline.clone();
+                let pipeline__ = pipeline.clone();
+                let callbacks_ = callbacks.clone();
+                let callbacks__ = callbacks.clone();


not too happy about this but this seems to make sense 😄

Yeah, I am not too happy either, but I couldn't think of a better approach :\

You could use a macro like clone_army! https://docs.rs/closet/0.2.2/closet/macro.clone_army.html

But it's all not very nice, not sure if this can be improved in any way though unless some kind of auto-cloning is added to the language (please no)

ferjm · 2018-06-19T07:51:47Z

Thanks for the review and feedback, folks!

sdroege · 2018-06-19T08:46:05Z

examples/audio_decoder.rs

+        .error(|| {
+            eprintln!("Error decoding audio");
+        })
+        .progress(move |buffer| {


Is there another user for this already apart from the example? Some code that does not actually copy all the samples directly but reuses the slice?

Not yet. I'm working on the DOM bindings using this code.

sdroege · 2018-06-19T08:46:58Z

servo-media/src/backends/gstreamer/audio_decoder.rs

+                let appsink = sink.clone().dynamic_cast::<AppSink>().map_err(|_| ())?;
+
+                // XXX Issue #18 channels support.
+                let audio_info = gst_audio::AudioInfo::new(gst_audio::AUDIO_FORMAT_F32, 44100, 1)


If 44100 Hz are not required, 48000 might be nicer... or can the sample rate even be signalled somewhere via webaudio so you wouldn't have to constrain it here?

Yeah, the sample rate should probably be given by the AudioContext. I'll write a patch for this.

You then want to set the caps similar to this (and possibly later also allow a different number of channels): https://github.com/sdroege/gstreamer-rs/blob/db3fe694154c697afdaf3efb6ec65332546942e0/examples/src/bin/appsink.rs#L54-L62

Basic audio decoder. Using audio sink to test for now

5a7986d

sdroege reviewed Jun 13, 2018

View changes

servo-media/src/backends/gstreamer/audio_decoder.rs Outdated

appsrc_

.push_buffer(buffer)

.into_result()

.expect("pushing buffer failed");

This comment has been minimized.

Sign in to view

sdroege Jun 13, 2018
Contributor

This should be cleanly handled instead of just panicking

sdroege reviewed Jun 13, 2018

View changes

ferjm added 5 commits Jun 14, 2018

Address review feedback: use Cursor, control sink addition and others

10d5a9b

AppSink. Test with channel based API. Still WIP. Lots of buffer copying

3db0890

AudioDecoderCallbacks

a24a899

Avoid extra buffer copy

37b3f42

Remove unused AudioDecoderMsg

2b8bda0

ferjm changed the title ~~[WIP] Audio decoder~~ Audio decoder Jun 18, 2018

sdroege reviewed Jun 18, 2018

View changes

Set AppSink caps

2c78c05

Manishearth approved these changes Jun 18, 2018

View changes

ferjm merged commit 5f16ef8 into servo:master Jun 19, 2018

ferjm deleted the ferjm:decoder branch Jun 19, 2018

sdroege reviewed Jun 19, 2018

View changes

ferjm mentioned this pull request Jun 20, 2018

Minor decoder fixes #53

Merged

servo / media

Audio decoder #52

Audio decoder #52

ferjm commented Jun 13, 2018 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm commented Jun 18, 2018

philn commented Jun 18, 2018

sdroege commented Jun 18, 2018

This comment has been minimized.

This comment has been minimized.

ferjm commented Jun 18, 2018

Manishearth left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm commented Jun 19, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

servo / media

Join GitHub today

Audio decoder #52

Audio decoder #52

Conversation

ferjm commented Jun 13, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm commented Jun 18, 2018

philn commented Jun 18, 2018

sdroege commented Jun 18, 2018

This comment has been minimized.

This comment has been minimized.

ferjm commented Jun 18, 2018

Manishearth left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm commented Jun 19, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm commented Jun 13, 2018 •

edited