GNOME Bugzilla – Bug 642730
New features for AAC and other passthrough support
Last modified: 2017-09-03 07:08:53 UTC
Created attachment 181304 [details] [review] [PATCH] [iec958] add a new plugin feature for AAC pass through output This patch adds the following features to iec958 plugin. basespdifenc: an abstract base class for IEC958 output, aac2spdif: an IEC958(IEC61937) payloader for AAC ADTS, aacspdifbin: an utility bin element which packages aacparse and aac2spdif, aacspdifsink: an utility sink bin of aacparse,aac2spdif,and alsasink, indented to be used for playbin2's audio-sink property.
Work is currently being done on making pulseaudio handle compressed formats, and then also playbin2. I'm not sure these convenience bins are the way forward. We need to do this more properly in both alsasink/audiosinks and playbin2, so that switching between audio tracks works fine etc. Maybe the payloading needs to be moved into the sinks (or convenience functions in libgstaudio/baseaudiosink or so). Having dedicated IEC958 payloaders just makes things overly complicated in playbin2, since these need to be special-cased then.
(In reply to comment #1) > Work is currently being done on making pulseaudio handle compressed formats, > and then also playbin2. That is an interesting and a good news for me. It would be nice that all the H/W decoded outputs (including VAAPI and VDPAU) are handled in a common way, and playbin2(3?) has a modified auto-plugging depending on the capabilities and configurations of sink elements. > I'm not sure these convenience bins are the way forward. We need to do this > more properly in both alsasink/audiosinks and playbin2, so that switching > between audio tracks works fine etc. Maybe the payloading needs to be moved > into the sinks (or convenience functions in libgstaudio/baseaudiosink or so). > Having dedicated IEC958 payloaders just makes things overly complicated in > playbin2, since these need to be special-cased then. Basically I agree with you, and that's why I gave those elements low ranks and created a sink-bin, which can be speficified by users outside the auto-plugging.
(In reply to comment #1) > I'm not sure these convenience bins are the way forward. We need to do this > more properly in both alsasink/audiosinks and playbin2, so that switching > between audio tracks works fine etc. Maybe the payloading needs to be moved > into the sinks (or convenience functions in libgstaudio/baseaudiosink or so). > Having dedicated IEC958 payloaders just makes things overly complicated in > playbin2, since these need to be special-cased then. I think it would be nice if audioconvert could accept ac3 or AAC, DTS... and convert to iec958, depending on sink's capability and user configurations, as the applications like totem or rhythmbox often prepend audioconvert to the real sink element, and I guess that audioconvert is already able to change its real src/sink caps depending on the caps of the sink.
After some more thinking about this a helper library for IEC958 payloading and using it inside the sinks is probably the better solution. You'll never need IEC958 anywhere else and separate elements are unecessary overhead for the trivial operation of IEC958 payloading. sinks could then simply require audio/x-aac for example and the payloading would happen internally. It would also allow sinks with special requirements to have special IEC958 payloading and would make sinks that really accept raw AAC/AC3 behave the same as sinks that internally require IEC958 payloading. This will also simplify the handling of all this inside playbin2/playsink (but of course still a lot of work is needed to make this all work without problems in all situations there). So the next step would be a small IEC958 payloading library, maybe as part of the audio library in gst-plugins-base and using it in alsasink for example.
Created attachment 185928 [details] [review] audio: Add an IEC 61937 payloading library This can be used by sinks to take compressed formats, correctly payload these in IEC 61937 frames and feed these to sinks that support passthrough output over IEC 60958 (S/PDIF) or, in the case of MP3, over Bluetooth. Initial implementation includes AC3, E-AC3, MPEG-1, MPEG-2 (non-AAC), and DTS (type-I/II/II) payloading. More formats can be added as needed.
Created attachment 185929 [details] [review] baseaudiosink: Allow subclasses to provide payloaders This allows subclasses to provide a "payload" function to prepare buffers for consumption. The immediate use for this is for sinks that can handle compressed formats - parsers are directly connected to the sink, and for formats such as AC3, DTS, and MPEG, IEC 61937 patyloading might be used.
I've got a tree (along with Sebastian's playbin2 work) to take this forward at http://git.collabora.co.uk/?p=user/arun/gst-plugins-base.git;a=shortlog;h=refs/heads/passthrough There's a bunch of small patches there to extend ringbuffer support for various formats and a couple of more intrusive changes that could use more eyeballs that I've attached here. The IEC 61937 payloader doesn't do AAC yet (I've no hardware that decodes AAC), but should be easy to extend. There's example code using this functionality at http://git.collabora.co.uk/?p=user/arun/gst-plugins-good.git;a=shortlog;h=refs/heads/passthrough (but I'm still rebasing this and tuning this a bit). If the patches attached (and in my -base tree) look good, I'll push to master post the 0.10.33 release.
Comment on attachment 181304 [details] [review] [PATCH] [iec958] add a new plugin feature for AAC pass through output Rejecting this patch because the payloading functionality should go in pulsesink/alsasink with Arun's patches
Review of attachment 185929 [details] [review]: ::: gst-libs/gst/audio/gstbaseaudiosink.c @@ +1716,3 @@ +done: + if (out) + gst_buffer_unref (out); Does this unref here make sense if it is done only for the case when payloading was done? Shouldn't that buffer always be unreffed or never? @@ +1733,3 @@ +payload_failed: + { + GST_DEBUG_OBJECT (sink, "failed to payload data for iec958 transmission"); This is not only for IEC958 I suppose? Just remove it from the debug output :) ::: gst-libs/gst/audio/gstbaseaudiosink.h @@ +145,3 @@ + * payloading is required, returns a reffed copy of the original + * buffer, else returns the payloaded buffer with all other metadata + * copied. Please add here that the subclass is supposed to take ownership of the passed buffer
Review of attachment 185928 [details] [review]: Looks good in general, just some minor things ::: gst-libs/gst/audio/gstaudioiec61937.c @@ +45,3 @@ + + /* We're assuming caps has only one structure. This should be fair since we + * don't expect a GstRingBufferSpec to carry buffers with multiple formats */ GstRingBufferSpec can only have fixed caps anyway ::: gst-libs/gst/audio/gstaudioiec61937.h @@ +2,3 @@ + * (c) 2011 Intel Corporation + * Collabora Multimedia + * Arun Raghavan <arun.raghavan@collabora.co.uk> We usually use Copyright (C) YEAR NAME and multiple lines for multiple copyright holders
(In reply to comment #9) > Review of attachment 185929 [details] [review]: Thanks for the review! > ::: gst-libs/gst/audio/gstbaseaudiosink.c > @@ +1716,3 @@ > +done: > + if (out) > + gst_buffer_unref (out); > > Does this unref here make sense if it is done only for the case when payloading > was done? Shouldn't that buffer always be unreffed or never? We need to keep this here because if we need to payload, we set buf = out and thus can't actually unref that buffer until we're done working with it. > @@ +1733,3 @@ > +payload_failed: > + { > + GST_DEBUG_OBJECT (sink, "failed to payload data for iec958 transmission"); > > This is not only for IEC958 I suppose? Just remove it from the debug output :) The error itself is useful when debugging problems. I'll remove the IEC958 bit. > ::: gst-libs/gst/audio/gstbaseaudiosink.h > @@ +145,3 @@ > + * payloading is required, returns a reffed copy of the original > + * buffer, else returns the payloaded buffer with all other metadata > + * copied. > > Please add here that the subclass is supposed to take ownership of the passed > buffer The subclass doesn't actually take ownership of the buffer. Or am I misunderstanding what you're saying?
(In reply to comment #10) > Review of attachment 185928 [details] [review]: > > Looks good in general, just some minor things Ack on both counts.
(In reply to comment #11) > (In reply to comment #9) [...] > > This is not only for IEC958 I suppose? Just remove it from the debug output :) > > The error itself is useful when debugging problems. I'll remove the IEC958 bit. Ah, the GST_ELEMENT_ERROR provides the actual error, so removing the debug print altogether.
(In reply to comment #0) > Created an attachment (id=181304) [details] [review] > [PATCH] [iec958] add a new plugin feature for AAC pass through output > > This patch adds the following features to iec958 plugin. > > basespdifenc: an abstract base class for IEC958 output, > aac2spdif: an IEC958(IEC61937) payloader for AAC ADTS, > aacspdifbin: an utility bin element which packages aacparse and aac2spdif, > aacspdifsink: an utility sink bin of aacparse,aac2spdif,and alsasink, > indented to be used for playbin2's audio-sink property. I've incorporated feedback and pushed the gstaudioiec61937 lib. If you redo these patches against this, I'm happy to review and push (I don't have any hardware that does AAC decode).
Instead of separate elements for IEC958 payloading it would be better to just use the new functions in alsasink for payloading and accept raw AAC/DTS/AC3.
(In reply to comment #15) > Instead of separate elements for IEC958 payloading it would be better to just > use the new functions in alsasink for payloading and accept raw AAC/DTS/AC3. I guess my comment wasn't clear - this is what I meant.
Arun, any updates on this ?
(In reply to comment #17) > Arun, any updates on this ? The original patches need to be ported into our iec61937 helper library (and 1.0), and then tested. I don't have any AAC decoder hardware, so unlikely to happen unless the reporter gets to it, or someone able to test this speaks up, in which case I can try to quickly do the port.
A patch for libgstaudio was already commited (#694443), but the one for pulsesink (#694445) is not.
Looks like this one is done now.