Discussion:
[Bug 765807] New: dvdlpcmdec: rewrite with GstAudioDecoder and add new format
"GStreamer" (GNOME Bugzilla)
2016-04-29 13:03:31 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

Bug ID: 765807
Summary: dvdlpcmdec: rewrite with GstAudioDecoder and add new
format
Classification: Platform
Product: GStreamer
Version: git master
OS: All
Status: NEW
Severity: enhancement
Priority: Normal
Component: gst-plugins-ugly
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@pengutronix.de
QA Contact: gstreamer-***@lists.freedesktop.org
GNOME version: ---

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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-04-29 13:04:15 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #1 from Michael Olbrich <***@pengutronix.de> ---
Created attachment 327020
--> https://bugzilla.gnome.org/attachment.cgi?id=327020&action=edit
tsdemux: stream_type 0x83 support
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-04-29 13:05:15 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #2 from Michael Olbrich <***@pengutronix.de> ---
Created attachment 327021
--> https://bugzilla.gnome.org/attachment.cgi?id=327021&action=edit
dvdlpcmdec: rewrite to use GstAudioDecoder
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-04-29 13:05:39 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #3 from Michael Olbrich <***@pengutronix.de> ---
Created attachment 327022
--> https://bugzilla.gnome.org/attachment.cgi?id=327022&action=edit
dvdlpcmdec: add support for another format
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-04-29 13:24:28 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #4 from Michael Olbrich <***@pengutronix.de> ---
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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-05-27 16:36:07 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

Tim-Philipp Müller <***@zen.co.uk> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@zen.co.uk
See Also| |https://bugzilla.gnome.org/
| |show_bug.cgi?id=703913
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-06-20 12:54:13 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

Michael Olbrich <***@pengutronix.de> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #327022|0 |1
is obsolete| |

--- Comment #5 from Michael Olbrich <***@pengutronix.de> ---
Created attachment 330067
--> https://bugzilla.gnome.org/attachment.cgi?id=330067&action=edit
dvdlpcmdec: add support for another format

A minor bugfix in gst_dvdlpcmdec_parse_1394():
4 bytes are read so 4 bytes should be mapped.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-06-20 12:56:19 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #6 from Michael Olbrich <***@pengutronix.de> ---
Created attachment 330068
--> https://bugzilla.gnome.org/attachment.cgi?id=330068&action=edit
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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-06-20 12:58:22 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #7 from Michael Olbrich <***@pengutronix.de> ---
Created attachment 330069
--> https://bugzilla.gnome.org/attachment.cgi?id=330069&action=edit
dvdlpcmdec: crop buffers with invalid size

This happens when a buffer is incomplete because of a lossy transport
layer like RTP.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-06-21 09:02:59 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #327021|none |reviewed
status| |

--- Comment #8 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 327021:
--> (https://bugzilla.gnome.org/review?bug=765807&attachment=327021)

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
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-06-21 09:03:25 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@coaxion.net
Attachment #327020|none |accepted-commit_now
status| |
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-06-21 09:04:13 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #330067|none |accepted-commit_now
status| |
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-06-21 09:06:04 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #330068|none |reviewed
status| |

--- Comment #9 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 330068:
--> (https://bugzilla.gnome.org/review?bug=765807&attachment=330068)

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
...?
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-06-21 09:06:53 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #330069|none |needs-work
status| |

--- Comment #10 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 330069:
--> (https://bugzilla.gnome.org/review?bug=765807&attachment=330069)

::: 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
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-06-21 09:08:18 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #11 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 330069:
--> (https://bugzilla.gnome.org/review?bug=765807&attachment=330069)

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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-07-15 12:58:04 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #12 from Michael Olbrich <***@pengutronix.de> ---
(In reply to Sebastian Dröge (slomo) from comment #8)
Post by "GStreamer" (GNOME Bugzilla)
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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-07-15 13:05:56 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #13 from Michael Olbrich <***@pengutronix.de> ---
(In reply to Sebastian Dröge (slomo) from comment #11)
Post by "GStreamer" (GNOME Bugzilla)
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?
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-07-18 08:13:25 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

Michael Olbrich <***@pengutronix.de> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #327021|reviewed |none
status| |
Attachment #327021|0 |1
is obsolete| |

--- Comment #14 from Michael Olbrich <***@pengutronix.de> ---
Created attachment 331679
--> https://bugzilla.gnome.org/attachment.cgi?id=331679&action=edit
dvdlpcmdec: rewrite to use GstAudioDecoder

Updated to only ref the buffer if needed.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-07-25 07:25:57 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #331679|none |accepted-commit_now
status| |
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-07-25 07:26:35 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #15 from Sebastian Dröge (slomo) <***@coaxion.net> ---
(In reply to Sebastian Dröge (slomo) from comment #9)
Post by "GStreamer" (GNOME Bugzilla)
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?
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-07-25 07:27:02 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #16 from Sebastian Dröge (slomo) <***@coaxion.net> ---
(In reply to Michael Olbrich from comment #13)
Post by "GStreamer" (GNOME Bugzilla)
(In reply to Sebastian Dröge (slomo) from comment #11)
Post by "GStreamer" (GNOME Bugzilla)
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
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-07-25 10:38:01 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #17 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Attachment 330067 pushed as 9f51f72 - dvdlpcmdec: add support for another
format
Attachment 331679 pushed as 35dc8d0 - dvdlpcmdec: rewrite to use
GstAudioDecoder
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-07-25 10:38:05 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #330067|accepted-commit_now |committed
status| |
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-07-25 10:38:09 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #331679|accepted-commit_now |committed
status| |
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-07-25 10:39:00 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #327020|accepted-commit_now |committed
status| |

--- Comment #18 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Comment on attachment 327020
--> https://bugzilla.gnome.org/attachment.cgi?id=327020
tsdemux: stream_type 0x83 support

commit 48c5cc1b1b5096ccff4a25c98e14117c6e5e4d25
Author: Michael 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
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-08-05 15:17:57 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #19 from Michael Olbrich <***@pengutronix.de> ---
(In reply to Sebastian Dröge (slomo) from comment #15)
Post by "GStreamer" (GNOME Bugzilla)
(In reply to Sebastian Dröge (slomo) from comment #9)
Post by "GStreamer" (GNOME Bugzilla)
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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-08-05 19:13:24 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

--- Comment #20 from Tim-Philipp Müller <***@zen.co.uk> ---
If you could attach a sample that'd be great.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2016-12-28 10:35:37 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807
Post by "GStreamer" (GNOME Bugzilla)
If you could attach a sample that'd be great.
Michael?
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
"GStreamer" (GNOME Bugzilla)
2018-11-03 15:35:34 UTC
Permalink
https://bugzilla.gnome.org/show_bug.cgi?id=765807

GStreamer system administrator <***@gstreamer.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |OBSOLETE

--- Comment #22 from GStreamer system administrator <***@gstreamer.net> ---
-- 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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
Loading...