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 665078 - opus: work without the Ogg headers
opus: work without the Ogg headers
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal normal
: 0.10.23
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 664817
 
 
Reported: 2011-11-28 19:48 UTC by Vincent Penquerc'h
Modified: 2011-12-12 12:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
opusdec: guard against decoding 0 samples (983 bytes, patch)
2011-11-28 19:48 UTC, Vincent Penquerc'h
committed Details | Review
opusdec: default to stereo 48000 Hz if possible when no headers seen (3.46 KB, patch)
2011-11-28 19:48 UTC, Vincent Penquerc'h
committed Details | Review
opusdec: Truncate caps first (1.29 KB, patch)
2011-12-07 05:08 UTC, Olivier Crête
committed Details | Review
opus: Special case the stereo behavior to disable the Ogg headers (2.23 KB, patch)
2011-12-07 05:50 UTC, Olivier Crête
none Details | Review
opus: Special case the stereo behavior to do a single stereo stream (2.09 KB, patch)
2011-12-07 06:02 UTC, Olivier Crête
reviewed Details | Review
opus: properly create channel mapping tables (18.22 KB, patch)
2011-12-08 18:50 UTC, Vincent Penquerc'h
committed Details | Review
opus: include streams count in caps (2.46 KB, patch)
2011-12-08 19:50 UTC, Vincent Penquerc'h
committed Details | Review
opusenc: add upstream negotiation for elements that can't deal with multistream (4.01 KB, patch)
2011-12-09 17:28 UTC, Vincent Penquerc'h
none Details | Review
opusenc: add upstream negotiation for elements that can't deal with multistream (4.10 KB, patch)
2011-12-09 17:48 UTC, Vincent Penquerc'h
none Details | Review
opusenc: add upstream negotiation for multistream ability (5.86 KB, patch)
2011-12-12 12:34 UTC, Vincent Penquerc'h
committed Details | Review

Description Vincent Penquerc'h 2011-11-28 19:48:42 UTC
opusdec: default to stereo 48000 Hz if possible when no headers seen
Comment 1 Vincent Penquerc'h 2011-11-28 19:48:44 UTC
Created attachment 202321 [details] [review]
opusdec: guard against decoding 0 samples
Comment 2 Vincent Penquerc'h 2011-11-28 19:48:47 UTC
Created attachment 202322 [details] [review]
opusdec: default to stereo 48000 Hz if possible when no headers seen
Comment 3 Sebastian Dröge (slomo) 2011-11-29 07:43:40 UTC
Comment on attachment 202321 [details] [review]
opusdec: guard against decoding 0 samples

This shouldn't really be needed but is of course a valid optimization :)
Comment 4 Sebastian Dröge (slomo) 2011-11-29 07:46:09 UTC
Comment on attachment 202322 [details] [review]
opusdec: default to stereo 48000 Hz if possible when no headers seen

Wouldn't it also make sense to get the rate/channels from the sinkpad caps if no header was provided in the caps and only fall back to 48khz/2channels if everything else fails?
Comment 5 Vincent Penquerc'h 2011-11-29 10:32:30 UTC
A single Opus stream can decode to 1 or 2 channels, the encoder is free to encode mono or stereo information based on the input, and the decoder can decode this to 1 or 2 channels, regardless of the input.
Caps are only set from headers, so if there's no header, there won't be caps, or those will be as good as the default since you can't look inside an Opus data packet and get more info than that (unless multistream, but these are only valid in Ogg and with headers).

I'm going to use that bug to keep track of shared patches with danilocesar, so will only commit this when we've made sure his RTP test case works, then I'll push.
Comment 6 Sebastian Dröge (slomo) 2011-12-05 09:14:05 UTC
You could get the caps from a capsfilter set by the application before opusdec for example
Comment 7 Olivier Crête 2011-12-07 05:08:05 UTC
Created attachment 202959 [details] [review]
opusdec: Truncate caps first

You also need to truncate the caps when you do the negotiation.
Comment 8 Olivier Crête 2011-12-07 05:50:33 UTC
Created attachment 202961 [details] [review]
opus: Special case the stereo behavior to disable the Ogg headers

For RTP, Opus does not support multistream. So special case it and don't do the evil
multi-stream header in these cases as it's not required.
Comment 9 Olivier Crête 2011-12-07 06:02:54 UTC
Created attachment 202962 [details] [review]
opus: Special case the stereo behavior to do a single stereo stream

For RTP, Opus does not support multistream. So special case it to make a single
stereo streams in that case
Comment 10 Olivier Crête 2011-12-07 06:18:52 UTC
We should probably add something with the numbers of streams to the audio/x-opus caps so that the payloader restricts it to a single stream.
Comment 11 Vincent Penquerc'h 2011-12-07 11:43:10 UTC
Comment on attachment 202959 [details] [review]
opusdec: Truncate caps first

Looks alright, but unrelated things (the #include stuff) that would be better in a separate one.
Comment 12 Vincent Penquerc'h 2011-12-07 11:49:33 UTC
Review of attachment 202962 [details] [review]:

::: ext/opus/gstopusenc.c
@@ +446,3 @@
       enc->channel_mapping_family = 0;
+      enc->channel_mapping[0] = 0;
+      enc->channel_mapping[0] = 1;

The mapping is not used for family 0 (particularity of the format).

@@ +551,3 @@
+      n_stereo_streams = 0;
+  } else {
+    n_streams = enc->n_channels;

That's just hardcoding for RTP.
It should be done for all cases when working out the channel positions.
At the moment I set it to all mono till I understand this bit, but just special casing the easy case means the general one is likely to get left undone...
Comment 13 Vincent Penquerc'h 2011-12-07 14:05:19 UTC
commit bab4c11b4ca5ec63133ea8f56f1c78a41c3fcf98
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Wed Dec 7 00:06:11 2011 -0500

    opusdec: header cleanup
    
    https://bugzilla.gnome.org/show_bug.cgi?id=665078

commit 094a02671e7fa4b4eae1eb38b8b46ef8c4769931
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Wed Dec 7 00:06:11 2011 -0500

    opusdec: Truncate caps first
    
    https://bugzilla.gnome.org/show_bug.cgi?id=665078

commit 9a7af231c739fd1fa426b74e997bea97275c9b90
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Mon Nov 28 19:47:34 2011 +0000

    opusdec: default to stereo 48000 Hz if possible when no headers seen
    
    https://bugzilla.gnome.org/show_bug.cgi?id=665078
Comment 14 Olivier Crête 2011-12-07 18:42:35 UTC
I agree it should auto-group all left-right pairs, but I was too lazy to do that, sorry :-(
Comment 15 Vincent Penquerc'h 2011-12-08 15:43:24 UTC
I'm back to this, I had a chat with the opus people to understand the mapping table (actually, the *two* tables and their interaction), and I think I understand it enough to do it right now. It's fiddly though. Wish me luck...
Comment 16 Vincent Penquerc'h 2011-12-08 18:50:33 UTC
Created attachment 203106 [details] [review]
opus: properly create channel mapping tables

There are two of them, unintuitively enough; the one passed
to the encoder should not be the one that gets written to the
file. The former maps the input to an ordering which puts
paired channels first, while the latter moves the channels
to Vorbis order. So add code to calculate both, and we now
have properly paired channels where appropriate.
Comment 17 Vincent Penquerc'h 2011-12-08 18:52:55 UTC
This patch should work with all inputs.
Tested with a couple 6 channel files with different orderings (though one of them is some "ambient", so I can't tell if there's any misordering on that one).
All left/right channels are now coupled, and this should include the canonical stereo mapping.
I'll leave that here till you confirm it works for you for the RTP case before pushing.
Comment 18 Olivier Crête 2011-12-08 19:01:48 UTC
Another addition I'd really like is to put the number of streams in the audio/x-opus caps so the rtp payloader can limit that to 1, this way we can make sure we always have something that will work. Otherwise the patch looks ok, did you test if it works with Empathy (pro tip: I use Empathy & Pidgin on the same machine for testing).
Comment 19 Vincent Penquerc'h 2011-12-08 19:28:25 UTC
It would make sense to add number of streams, yes, since that's at the framing level.

I did not test with empathy, I thought you were doing just that. I've no idea how to set that up and get it to use my gst.
Comment 20 Vincent Penquerc'h 2011-12-08 19:50:01 UTC
Created attachment 203109 [details] [review]
opus: include streams count in caps
Comment 21 Olivier Crête 2011-12-09 01:10:06 UTC
Any chance you can also add that to the upstream caps negotiation somehow.. Like limit the number of channels to streams / 2 in the getcaps().
Comment 22 Vincent Penquerc'h 2011-12-09 11:19:56 UTC
I'd have imagined you'd insert a capsfilter downstream of opusenc or opusparse when you want that behavior.
Limiting the number of streams to (channels+1)/2 would eliminate the ability to pair the relevant channels in all cases. Or am I misunderstandind your point ?
Comment 23 Olivier Crête 2011-12-09 15:09:27 UTC
My use-case is very simple, I want to limit the numbers of streams to 1 so it works with RTP and that means that the source has to be mono or stereo. So I want these caps restrictions on rtpopuspay. But limiting to 2 channels isn't enough, I need to limit to 2 paired channels at most.
Comment 24 Vincent Penquerc'h 2011-12-09 15:14:15 UTC
This is achieved easily by adding a capsfilter specifying "streams=1", no ?
Or adding "streams=1" to the caps of the RTP payloader. But not in opusenc itself, as this restriction is only needed for the RTP elements, not for the general case. Maybe I'm missing your point though ?
Comment 25 Olivier Crête 2011-12-09 15:45:43 UTC
My point is if I have "audiosrc ! opusenc ! rtpopuspay", it should always produce something valid.. So audiosrc should know that it can only do a mono stream or a valid stereo stream.
Comment 26 Vincent Penquerc'h 2011-12-09 16:22:18 UTC
Oh, I see, forward upstream a channel constraint based on the stream constraint from downstream. Makes sense.

Buggily, I tried setting a streams=1 caps constraint on rtpopuspay and it's linking happily to opusenc which creates caps with streams=4 :S

Looking...
Comment 27 Vincent Penquerc'h 2011-12-09 17:28:35 UTC
Created attachment 203160 [details] [review]
opusenc: add upstream negotiation for elements that can't deal with multistream

Such as the RTP payloader.
Comment 28 Vincent Penquerc'h 2011-12-09 17:31:53 UTC
Channel positions aren't included, as AFAIK there's no way to say "this property should not be present" in caps. So it could theoretically happen that we'll get an input of 2 channels being center and side right, but what's the chance ? :)
I don't think I've ever seen channel positions on 1 or 2 channels in gst. But it's a theoretically possible case.

Piping to rtpopusenc now causes the opusenc sink caps to advertise channels of [1, 2] instead of [1, 8].
Comment 29 Vincent Penquerc'h 2011-12-09 17:48:23 UTC
Created attachment 203162 [details] [review]
opusenc: add upstream negotiation for elements that can't deal with multistream

Such as the RTP payloader.
Comment 30 Vincent Penquerc'h 2011-12-12 12:34:25 UTC
Created attachment 203246 [details] [review]
opusenc: add upstream negotiation for multistream ability

This will help elements that cannot deal with multistream,
such as the RTP payloader.

The caps now do not include a "streams" field anymore, but
a "multistream" boolean, since we have no real use for knowing
the exact amount of streams.
Comment 31 Vincent Penquerc'h 2011-12-12 12:35:49 UTC
commit f40ccb381166fc86eec1fe8003992b41a814a16c
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Fri Dec 9 17:25:41 2011 +0000

    opusenc: add upstream negotiation for multistream ability
    
    This will help elements that cannot deal with multistream,
    such as the RTP payloader.
    
    The caps now do not include a "streams" field anymore, but
    a "multistream" boolean, since we have no real use for knowing
    the exact amount of streams.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=665078