After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 689460 - Can't playback LPCM data via HTTP after playing back something else
Can't playback LPCM data via HTTP after playing back something else
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.x
Other Linux
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-12-01 22:41 UTC by Jens Georg
Modified: 2016-05-15 10:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Self-contained example (564.41 KB, application/x-xz)
2012-12-05 21:14 UTC, Jens Georg
  Details
Patch to change audio/x-raw to audio/L16 in souphttpsrc (1.27 KB, patch)
2016-03-03 15:56 UTC, Carlos Rafael Giani
none Details | Review
Adds an L16 parser to rawparse (9.86 KB, patch)
2016-03-03 15:57 UTC, Carlos Rafael Giani
none Details | Review
Patch to change audio/x-raw to audio/x-unaligned-raw in souphttpsrc (1.23 KB, patch)
2016-03-04 21:18 UTC, Carlos Rafael Giani
committed Details | Review
Adds unaligned raw audio data parsing (11.77 KB, patch)
2016-03-04 21:23 UTC, Carlos Rafael Giani
committed Details | Review

Description Jens Georg 2012-12-01 22:41:16 UTC
Situation as follows:

1) Playback audio (MP3, LPCM,...) on a pristine playbin via http
2) Setting playbin state to READY.
3) Setting playbin.uri to a new LPCM audio uri on http. The Content-Type definitely is correct (audio/L16;rate=44100;channels=2), can even be the same track that worked perfectly fine in 1)
4) Setting playbin state to PLAYING
5) Critical on console, no audio played:

(lt-rygel:2148): GStreamer-CRITICAL **: gst_segment_to_running_time: assertion `segment->format == format' failed

This seems to work with 0.10.

Trace:

  • #0 g_logv
    at gmessages.c line 853
  • #1 g_log
    at gmessages.c line 1003
  • #2 gst_segment_to_running_time
    at gstsegment.c line 476
  • #3 gst_audio_base_sink_render
    at gstaudiobasesink.c line 1757
  • #4 gst_base_sink_chain_unlocked
    at gstbasesink.c line 3319
  • #5 gst_base_sink_chain_main
    at gstbasesink.c line 3427
  • #6 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #7 gst_pad_push_data
    at gstpad.c line 3871
  • #8 gst_proxy_pad_chain_default
    at gstghostpad.c line 128
  • #9 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #10 gst_pad_push_data
    at gstpad.c line 3871
  • #11 gst_proxy_pad_chain_default
    at gstghostpad.c line 128
  • #12 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #13 gst_pad_push_data
    at gstpad.c line 3871
  • #14 gst_base_transform_chain
    at gstbasetransform.c line 2203
  • #15 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #16 gst_pad_push_data
    at gstpad.c line 3871
  • #17 gst_base_transform_chain
    at gstbasetransform.c line 2203
  • #18 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #19 gst_pad_push_data
    at gstpad.c line 3871
  • #20 gst_proxy_pad_chain_default
    at gstghostpad.c line 128
  • #21 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #22 gst_pad_push_data
    at gstpad.c line 3871
  • #23 gst_queue_push_one
    at gstqueue.c line 1080
  • #24 gst_queue_loop
    at gstqueue.c line 1196
  • #25 gst_task_func
    at gsttask.c line 316
  • #26 g_thread_pool_thread_proxy
    at gthreadpool.c line 309
  • #27 g_thread_proxy
    at gthread.c line 798
  • #28 start_thread
    at pthread_create.c line 308
  • #29 clone
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 112
  • #30 ??

Comment 1 Tim-Philipp Müller 2012-12-04 23:51:09 UTC
Can you provide a public HTTP URI with such LPCM audio so I can test this with?

Could you attach a (gzipped, if needed) GST_DEBUG=*:6 debug log please? 

I don't really known how this (audio/L16 content-type) can work properly tbh. souphttpsrc just puts raw audio caps on buffers for audio/L16, but that's not really right and assumes the chunking will line up etc. And souphttpsrc will produce a BYTE segment.
Comment 2 Jens Georg 2012-12-05 21:12:35 UTC
Logfile is at http://rygel-project.org/snapshot/testfiles/pcm-test.log.xz
Comment 3 Jens Georg 2012-12-05 21:14:24 UTC
Created attachment 230827 [details]
Self-contained example

Sample code with HTTP server, sample audio file and playbin.
Comment 4 Jens Georg 2012-12-05 21:14:39 UTC
info provided
Comment 5 Jens Georg 2012-12-05 21:17:17 UTC
sample data is audio/x-raw-int,channels=2,width=16,depth=16,signed=true,endianness=4321,rate=44100
Comment 6 Sebastian Dröge (slomo) 2013-12-23 15:18:00 UTC
Confirmed here, the problem is also that souphttpsrc does not necessarily return data in chunks that are a multiple of the sample size:
** (lt-pcm-test:11512): WARNING **: conv: size 3967 is not a multiple of unit size 4

The segment warnings come from the fact that souphttpsrc does not output a TIME segment. And baseaudiosink assumes TIME format.

I think this use case is broken by design, there should be an element after the source that properly timestamps and chunks the data.
Comment 7 Tim-Philipp Müller 2013-12-24 00:40:24 UTC
I guess we need a way to autoplug an audioparse..
Comment 8 Michael Pujos 2014-02-09 18:32:10 UTC
This bug breaks playback of audio/L16 (aka LPCM) in gmrender-resurrect:

https://github.com/hzeller/gmrender-resurrect/issues/14

audio/L16 is used frequently in UPnP/DLNA streaming. In particular, it is a mandatory format for a device to be DLNA certified. 
When an UPnP/DLNA device doesn't support a particular audio codec, it is frequent that media servers transcode to audio/L16. It is also used by media servers that generate realtime audio streams, like the output of an audio card.

That's why I think fixing this bug is important.

As a side not, this mutiple of 4 issue doesn't seem to happen on x86. 
But it sure do happen on ARM, for example on Raspberry Pi.
I suppose that on x86, read() always return data whose size is multiple of 4.
Comment 9 Olivier Crête 2014-09-08 16:41:50 UTC
Maybe a quick option would be to add a audio/x-unaligned-raw format, have sources like souphttpsrc/dlnasrc produce that instead of audio/x-raw (because it's not proper audio/x-raw since the samples are cut), then create a subclass of audioparse that only takes audio/x-unaligned-raw as input and audio/x-raw as output and rank that... Or modify demuxers to add a aligned=TRUE to audio/x-raw formats and just rank audioparse ?
Comment 10 Carlos Rafael Giani 2016-03-03 11:58:43 UTC
I favor the media type idea. But, this seems L16 specific. So instead of audio/x-unaligned-raw I'd just set audio/L16.
Comment 11 Carlos Rafael Giani 2016-03-03 15:56:25 UTC
Created attachment 322999 [details] [review]
Patch to change audio/x-raw to audio/L16 in souphttpsrc

This patch modifies the caps that souphttpsrc sets when encountering L16 data. With audio/L16, an L16 parser can be autoplugged.
Comment 12 Carlos Rafael Giani 2016-03-03 15:57:20 UTC
Created attachment 323002 [details] [review]
Adds an L16 parser to rawparse

This adds an L16 parser to the rawparse plugin. It is a bin, can be autoplugged, and internally uses audioparse, configured to output S16BE data.
Comment 13 Sebastian Dröge (slomo) 2016-03-04 07:34:13 UTC
Review of attachment 323002 [details] [review]:

This looks like you want to run gst-indent over the .c files.

::: gst/rawparse/gstL16parse.c
@@ +42,3 @@
+
+
+G_DEFINE_TYPE (GstL16Parse, gst_L16_parse, GST_TYPE_BIN)

Probably gst-indent after adding a ; at the end of the line here

@@ +85,3 @@
+      gst_element_factory_make ("audioparse", "L16_inner_parser");
+  if (L16_parse->inner_parser == NULL)
+    return;

You can make this an assertion. It should always work.

@@ +88,3 @@
+
+  g_object_set (G_OBJECT (L16_parse->inner_parser),
+      "format", (gint) 0,

The enums are in the same plugin, so use them :)

@@ +187,3 @@
+
+      gst_query_parse_caps (query, &filter);
+      caps = gst_pad_get_pad_template_caps (GST_PAD (pad));

Get the downstream caps (downstream of audioparse I guess) intersected with your template caps, and only if there is no downstream use the template caps instead.
Comment 14 Sebastian Dröge (slomo) 2016-03-04 07:35:43 UTC
Comment on attachment 322999 [details] [review]
Patch to change audio/x-raw to audio/L16 in souphttpsrc

This is strictly speaking a breaking change. But it should finally fix this situation properly... OTOH might break existing applications, and now requires a -bad plugin for proper functioning.

Opinions?
Comment 15 Carlos Rafael Giani 2016-03-04 07:43:59 UTC
Come to think of it, while the main use case right now is L16, having it more generic for unaligned raw data may make sense in the future, also because audioparse provides seeking, right?

Also, I did run gst-indent over these files! Apparently it didn't catch everything.

And yeah, it is a breaking change. But I think requiring a "parsed" field in the audio/x-raw caps would be worse, because it would have to be introduced in many other elements. Unless of course we assume "parsed=TRUE" to be implicit, and only if "parsed" is FALSE, then a raw parser would kick in. This is unusual though - typically, parsers only get autoplugged if "parsed=TRUE" is missing. So, this would be an inconsistency.
Comment 16 Sebastian Dröge (slomo) 2016-03-04 07:57:44 UTC
(In reply to Carlos Rafael Giani from comment #15)
> Come to think of it, while the main use case right now is L16, having it
> more generic for unaligned raw data may make sense in the future, also
> because audioparse provides seeking, right?

Yes, but maybe audioparse (and videoparse) should already default to using the caps event to configure themselves (or have a property?).

> Also, I did run gst-indent over these files! Apparently it didn't catch
> everything.

You're missing a semicolon after G_DEFINE_TYPE. That's why it is all confused

> And yeah, it is a breaking change. But I think requiring a "parsed" field in
> the audio/x-raw caps would be worse, because it would have to be introduced
> in many other elements. Unless of course we assume "parsed=TRUE" to be
> implicit, and only if "parsed" is FALSE, then a raw parser would kick in.
> This is unusual though - typically, parsers only get autoplugged if
> "parsed=TRUE" is missing. So, this would be an inconsistency.

Yes, also it barely worked before without having a parser :)
Comment 17 Tim-Philipp Müller 2016-03-04 09:05:06 UTC
The scenario that we must not break is people doing a manual pipeline with souphttpsrc ! audioparse
Comment 18 Carlos Rafael Giani 2016-03-04 21:18:14 UTC
Created attachment 323130 [details] [review]
Patch to change audio/x-raw to audio/x-unaligned-raw in souphttpsrc

New souphttpsrc patch. The same as the old one, except that it uses the more generic audio/x-unaligned-raw caps instead of audio/L16.
Comment 19 Carlos Rafael Giani 2016-03-04 21:23:06 UTC
Created attachment 323131 [details] [review]
Adds unaligned raw audio data parsing

This adds the ability to parse audio/x-unaligned-raw data. audioparse accepts audio/x-unaligned-raw. If "use-sink-caps" is set to TRUE, the input caps are used as the output caps, except that the name "audio/x-unaligned-raw" is replaced with "audio/x-raw". This ensures that "souphttpsrc ! audioparse" still works.

The new unalignedaudioparse bin sets up an audioparse element with "use-sink-caps" set to TRUE. Due to audioparse's new support for the unaligned caps, the code of unalignedaudioparse becomes very small; the former event and query handlers are now unnecessary.
Comment 20 Sebastian Dröge (slomo) 2016-03-05 08:17:43 UTC
(In reply to Tim-Philipp Müller from comment #17)
> The scenario that we must not break is people doing a manual pipeline with
> souphttpsrc ! audioparse

That will always work as audioparse accepts anything. But what might be a problem is people who only dynamically add the audioparse if the caps on souphttpsrc are audio/x-raw. That's my main worry.
Comment 21 Carlos Rafael Giani 2016-03-05 09:29:01 UTC
(In reply to Sebastian Dröge (slomo) from comment #20)
> (In reply to Tim-Philipp Müller from comment #17)
> > The scenario that we must not break is people doing a manual pipeline with
> > souphttpsrc ! audioparse
> 
> That will always work as audioparse accepts anything. But what might be a
> problem is people who only dynamically add the audioparse if the caps on
> souphttpsrc are audio/x-raw. That's my main worry.

Agreed, but I cannot see a way around it. Introducing autoplugging for audio/x-raw would potentially break a lot of other cases. There is probably no 100% clean way out of this.
Comment 22 Tim-Philipp Müller 2016-03-05 09:39:29 UTC
If there was an audioparse variant for autoplugging it could simply have an additional aligned=false or such in its template caps, which means it would never be plugged for any normal raw audio caps (besides, decodebin would stop already at that point anyway).
Comment 23 Carlos Rafael Giani 2016-03-05 10:03:00 UTC
(In reply to Tim-Philipp Müller from comment #22)
> If there was an audioparse variant for autoplugging it could simply have an
> additional aligned=false or such in its template caps, which means it would
> never be plugged for any normal raw audio caps (besides, decodebin would
> stop already at that point anyway).

But caps with "parsed=FALSE" are a subset of caps with no "parsed" field, aren't they? How would autoplugging work with this? Also, this sort of reverses the typical usage of "parsed" caps. Usually, "parsed=FALSE" is implicit.
Comment 24 Sebastian Dröge (slomo) 2016-03-05 10:03:23 UTC
You mean aligned=false for audio/x-raw? That won't work: decodebin intersects audio/x-raw,whatever with audio/x-raw, non-empty result, decides it's raw caps and exposes the pad.
Comment 25 Tim-Philipp Müller 2016-03-05 10:14:54 UTC
Ah, it doesn't do a subset check then, ok.
Comment 26 Christian Weiske 2016-04-18 10:50:05 UTC
I ran into this bug while trying to make a UPnP media renderer on top of a Raspberry Pi - see http://cweiske.de/tagebuch/gmediarenderer.htm
I'd really like to see this fixed since it prevents me from using my .ogg music library.
Comment 27 Sebastian Dröge (slomo) 2016-04-18 11:34:28 UTC
If you want to do DLNA, a plain HTTP source is not going to be enough and you need special handling anyway. In your application you could use the audioparse element with correct settings here for example, or make a custom source for DLNA content. DLNA is not HTTP, even if it pretends that.
Comment 28 Carlos Rafael Giani 2016-04-18 11:38:55 UTC
Note though that my patches would autoplug everything, and no special handling would be necessary.
Comment 29 Sebastian Dröge (slomo) 2016-04-18 11:40:32 UTC
For one specific case that we should handle, yes. But not for DLNA in general.
Comment 30 Carlos Rafael Giani 2016-04-18 11:41:39 UTC
Okay. I will re-check my patches, and then request here that they are reviewed for inclusion into the repositories.
Comment 31 Carlos Rafael Giani 2016-05-02 11:59:09 UTC
I think the patches are valid. Can we review them again for 1.10 ?
Comment 32 Sebastian Dröge (slomo) 2016-05-03 07:54:55 UTC
Review of attachment 323131 [details] [review]:

Mostly looks good to me, just cosmetical below. Also it's a bit unfortunate that this now requires a -bad plugin but what can we do other than moving it asap :)

I'll keep it in bugzilla for another day or two, in case someone else has opinions

::: gst/rawparse/Makefile.am
@@ +22,2 @@
 noinst_HEADERS = \
+	unalignedaudio.h \

Why not just put that into the gstunalignedaudioparse.h header, gstrawparse.h or other :)
Comment 33 Carlos Rafael Giani 2016-05-03 08:00:56 UTC
> ::: gst/rawparse/Makefile.am
> @@ +22,2 @@
>  noinst_HEADERS = \
> +	unalignedaudio.h \
> 
> Why not just put that into the gstunalignedaudioparse.h header,
> gstrawparse.h or other :)

I didn't want to include the header of one element (unalignedaudioparse) into the header of another element (audioparse) just for the GST_UNALIGNED_RAW_AUDIO_CAPS definition. So instead, I factored it out and made both audioparse and unalignedaudioparse dependent on that.

And you are right, it should be moved. However, I do think this plugin needs an overhaul. It isn't based on baseparse, and apparently does the seeking badly.
Comment 34 Sebastian Dröge (slomo) 2016-05-03 08:04:49 UTC
(In reply to Carlos Rafael Giani from comment #33)
> > ::: gst/rawparse/Makefile.am
> > @@ +22,2 @@
> >  noinst_HEADERS = \
> > +	unalignedaudio.h \
> > 
> > Why not just put that into the gstunalignedaudioparse.h header,
> > gstrawparse.h or other :)
> 
> I didn't want to include the header of one element (unalignedaudioparse)
> into the header of another element (audioparse) just for the
> GST_UNALIGNED_RAW_AUDIO_CAPS definition. So instead, I factored it out and
> made both audioparse and unalignedaudioparse dependent on that.

Alright then :)

> And you are right, it should be moved. However, I do think this plugin needs
> an overhaul. It isn't based on baseparse, and apparently does the seeking
> badly.

Yes, it's older than baseparse. Porting it to baseparse should be absolutely trivial though as rawparse has a constant frame size. It should be mostly a matter of removing code. Are you going to take a look at that?
Comment 35 Carlos Rafael Giani 2016-05-03 08:05:31 UTC
OK, I can try.
Comment 36 Tim-Philipp Müller 2016-05-03 08:25:29 UTC
This still breaks backwards compatibility for people who wait for caps on souphttpsrc before deciding whether to plug audioparse or decodebin, and we know people do that, because they have to do that currently...
Comment 37 Sebastian Dröge (slomo) 2016-05-03 08:27:19 UTC
You mean it breaks because the caps are different now? I don't see a way around that. Either we break that, or it's not going to work out of the box.
Comment 38 Tim-Philipp Müller 2016-05-03 08:44:35 UTC
The way around that is to do chunking in souphttpsrc, ugly as it may be.
Comment 39 Sebastian Dröge (slomo) 2016-05-03 08:50:23 UTC
Right, that would be the ugly solution. Or special casing in decodebin that if it gets raw audio data, it should first plug something else.
Comment 40 Carlos Rafael Giani 2016-05-03 08:53:34 UTC
There's another option: we turn the raw parsing into a library, rewrite the rawparse plugin to be based on baseparse+this library, and integrate the library into souphttpsrc for chunking.
Comment 41 Carlos Rafael Giani 2016-05-03 08:58:55 UTC
Or, yet another option: I've read opinions that souphttpsrc needs to be rewritten on top of the new Soup API (not sure what that is, I don't know Soup so well). So, one way would be to develop a souphttpsrc2 ..
Comment 42 Sebastian Dröge (slomo) 2016-05-06 06:27:15 UTC
So what should we do here? IMHO it's just wrong that souphttpsrc claims to produce audio/x-raw, it's providing the wrong caps. That's as wrong as a pipeline doing filesrc ! "video/x-raw" ! ...
Comment 43 Sebastian Dröge (slomo) 2016-05-06 06:44:47 UTC
(In reply to Carlos Rafael Giani from comment #41)
> Or, yet another option: I've read opinions that souphttpsrc needs to be
> rewritten on top of the new Soup API (not sure what that is, I don't know
> Soup so well). So, one way would be to develop a souphttpsrc2 ..

See https://bugzilla.gnome.org/show_bug.cgi?id=693911
Comment 44 Carlos Rafael Giani 2016-05-06 07:15:08 UTC
(In reply to Tim-Philipp Müller from comment #36)
> This still breaks backwards compatibility for people who wait for caps on
> souphttpsrc before deciding whether to plug audioparse or decodebin, and we
> know people do that, because they have to do that currently...

It occurred to me that the audio/x-unaligned-raw caps actually do *not* break anything, because in the case you described, the code which waits for caps on souphttpsrc before deciding whether or not to manually add audioparse will expect the audio/x-raw caps in order to do that. So, if souphttpsrc doesn't ever set audio/x-raw caps as the media type again, then this code will just never manually plug in audioparse, and always rely on decodebin instead, which will autoplug everything properly.

So, the one case that would be broken is if souphttpsrc is used *only* for audio/x-raw data, and for some reason the code checks the media type without using decodebin as a fallback.
Comment 45 Tim-Philipp Müller 2016-05-13 09:43:41 UTC
(In reply to Carlos Rafael Giani from comment #44)

That's true. I think I can live with that corner case breaking, I am not aware of anyone doing that. We'll just have to mention it in the release notes.
Comment 46 Sebastian Dröge (slomo) 2016-05-15 10:26:53 UTC
commit b415d7b34f5a5d030165f2596e7074cc45bd48a4
Author: Carlos Rafael Giani <dv@pseudoterminal.org>
Date:   Fri Mar 4 22:10:47 2016 +0100

    rawparse: Add unaligned raw audio parsing to audioparse and add new element
    
    This helps in cases where raw audio data is being delivered, but the
    buffers do not come in sample aligned sizes. The new unalignedaudioparse
    bin can be autoplugged and configures an internal audioparse element to
    align the data. audioparse itself gets support for audio/x-unaligned-raw
    input caps; the output caps then contain the same information, except that
    the name is changed to audio/x-raw (since audioparse aligns the data).
    This ensures that souphttpsrc ! audioparse still works.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=689460


commit e9a795fa8bea7aee6fd15b9865744b61b22c2435
Author: Carlos Rafael Giani <dv@pseudoterminal.org>
Date:   Fri Mar 4 10:14:00 2016 +0100

    souphttpsrc: Use audio/x-unaligned-raw instead of audio/x-raw for L16 data
    
    Directly setting audio/x-raw caps leads to problems when the delivered
    data blocks do not align properly at sample boundaries (for example, a
    data block with 391 bytes). So, instead, set audio/x-unaligned-raw to
    let a parser be autoplugged.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=689460