GNOME Bugzilla – Bug 765807
dvdlpcmdec: rewrite with GstAudioDecoder and add new format
Last modified: 2018-11-03 15:35:34 UTC
There are multiple variations of LPCM. One for DVDs is already implemented in dvdlpcmdec. Another is defined in the "DVD-Video/Audio through IEEE1394 Bus" spec (http://www.dvdforum.org/images/Guideline1394V10R0_20020911.pdf). This was also reused for Wifi-Display. I have 3 Patches to support this: 1. Add support for the stream_type (0x83) in tsdemux 2. Convert dvdlpcmdec to use GstAudioDecoder 3. Add support for the format mentioned above.
Created attachment 327020 [details] [review] tsdemux: stream_type 0x83 support
Created attachment 327021 [details] [review] dvdlpcmdec: rewrite to use GstAudioDecoder
Created attachment 327022 [details] [review] dvdlpcmdec: add support for another format
I've tested what I could with my streams and the files from https://samples.ffmpeg.org/MPEG-VOB/LPCM/ to make sure I didn't break anything. However, I don't have a test-case for width == 20 so that part is untested.
Created attachment 330067 [details] [review] dvdlpcmdec: add support for another format A minor bugfix in gst_dvdlpcmdec_parse_1394(): 4 bytes are read so 4 bytes should be mapped.
Created attachment 330068 [details] [review] dvdlpcmdec: add support for yet another format I've looked at the sample in #703913. That's yet another format. Here's a patch for it.
Created attachment 330069 [details] [review] dvdlpcmdec: crop buffers with invalid size This happens when a buffer is incomplete because of a lossy transport layer like RTP.
Review of attachment 327021 [details] [review]: Generally looks good, thanks! Good work ::: gst/dvdlpcmdec/gstdvdlpcmdec.c @@ +544,3 @@ channels = GST_AUDIO_INFO_CHANNELS (&dvdlpcmdec->info); + gst_buffer_ref (buf); Why? Please make sure it is unreffed in all cases, but this seems unnecessary? The base class keeps track of that for you
Review of attachment 330068 [details] [review]: This seems unfortunate. Can't we somehow detect this at the MPEG-TS level and give it different caps? Is there some kind of different descriptor for it or ...?
Review of attachment 330069 [details] [review]: ::: gst/dvdlpcmdec/gstdvdlpcmdec.c @@ +681,3 @@ + if (size % (channels * 2)) { + GST_WARNING_OBJECT (dvdlpcmdec, "Buffer of size %" G_GSIZE_FORMAT + " is not a multiple of %d. Droping incomplete sample", Typo: Dropping with double p
Review of attachment 330069 [details] [review]: And even in a lossy transport like RTP, how would you end up with incomplete packets? At the UDP level this should've already been dropped if the UDP packet was incomplete, and the RTP depayloader should detect if there are missing packets and handle that somehow.
(In reply to Sebastian Dröge (slomo) from comment #8) > Review of attachment 327021 [details] [review] [review]: > > Generally looks good, thanks! Good work > > ::: gst/dvdlpcmdec/gstdvdlpcmdec.c > @@ +544,3 @@ > channels = GST_AUDIO_INFO_CHANNELS (&dvdlpcmdec->info); > > + gst_buffer_ref (buf); > > Why? Please make sure it is unreffed in all cases, but this seems > unnecessary? The base class keeps track of that for you That's because of the new base class. This code was the pad chain function so it owned the buffer. With the new base class it's in handle_frame which does not own the buffer. There are cases where the buffer is passed to gst_audio_decoder_finish_frame() because there is nothing to do. In all other cases the buffer is eventually unrefed again. I can change this to only ref the buffer if necessary. Maybe that will be clearer.
(In reply to Sebastian Dröge (slomo) from comment #11) > Review of attachment 330069 [details] [review] [review]: > > And even in a lossy transport like RTP, how would you end up with incomplete > packets? At the UDP level this should've already been dropped if the UDP > packet was incomplete, and the RTP depayloader should detect if there are > missing packets and handle that somehow. The pipeline here is basically rtpmp2tdepay -> tsdemux -> dvdlpcmdec. I don't think rtpmp2tdepay can handle this. Maybe tsdemux can, but I'm not sure it should: That would just mean we loose more data. This way we play as much as possible. But I should add a GST_BUFFER_FLAG_DISCONT to the next buffer, right?
Created attachment 331679 [details] [review] dvdlpcmdec: rewrite to use GstAudioDecoder Updated to only ref the buffer if needed.
(In reply to Sebastian Dröge (slomo) from comment #9) > Review of attachment 330068 [details] [review] [review]: > > This seems unfortunate. Can't we somehow detect this at the MPEG-TS level > and give it different caps? Is there some kind of different descriptor for > it or ...? How should we go ahead with this?
(In reply to Michael Olbrich from comment #13) > (In reply to Sebastian Dröge (slomo) from comment #11) > > Review of attachment 330069 [details] [review] [review] [review]: > > > > And even in a lossy transport like RTP, how would you end up with incomplete > > packets? At the UDP level this should've already been dropped if the UDP > > packet was incomplete, and the RTP depayloader should detect if there are > > missing packets and handle that somehow. > > The pipeline here is basically rtpmp2tdepay -> tsdemux -> dvdlpcmdec. I > don't think rtpmp2tdepay can handle this. Maybe tsdemux can, but I'm not > sure it should: That would just mean we loose more data. This way we play as > much as possible. But I should add a GST_BUFFER_FLAG_DISCONT to the next > buffer, right? Yes, seems ok then
Attachment 330067 [details] pushed as 9f51f72 - dvdlpcmdec: add support for another format Attachment 331679 [details] pushed as 35dc8d0 - dvdlpcmdec: rewrite to use GstAudioDecoder
Comment on attachment 327020 [details] [review] tsdemux: stream_type 0x83 support commit 48c5cc1b1b5096ccff4a25c98e14117c6e5e4d25 Author: Michael Olbrich <m.olbrich@pengutronix.de> Date: Fri Apr 29 14:42:34 2016 +0200 tsdemux: add support for LPCM with stream_type = 0x83 https://bugzilla.gnome.org/show_bug.cgi?id=765807
(In reply to Sebastian Dröge (slomo) from comment #15) > (In reply to Sebastian Dröge (slomo) from comment #9) > > Review of attachment 330068 [details] [review] [review] [review]: > > > > This seems unfortunate. Can't we somehow detect this at the MPEG-TS level > > and give it different caps? Is there some kind of different descriptor for > > it or ...? > > How should we go ahead with this? I don't know. I didn't see any difference at MPEG-TS level when I implemented this. But I'm not an expert when it comes to MPEG-TS. I can provide a sample of the other format if someone else wants to take a look at this.
If you could attach a sample that'd be great.
> If you could attach a sample that'd be great. Michael?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-ugly/issues/15.