GNOME Bugzilla – Bug 665078
opus: work without the Ogg headers
Last modified: 2011-12-12 12:35:49 UTC
opusdec: default to stereo 48000 Hz if possible when no headers seen
Created attachment 202321 [details] [review] opusdec: guard against decoding 0 samples
Created attachment 202322 [details] [review] opusdec: default to stereo 48000 Hz if possible when no headers seen
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 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?
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.
You could get the caps from a capsfilter set by the application before opusdec for example
Created attachment 202959 [details] [review] opusdec: Truncate caps first You also need to truncate the caps when you do the negotiation.
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.
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
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 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.
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...
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
I agree it should auto-group all left-right pairs, but I was too lazy to do that, sorry :-(
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...
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.
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.
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).
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.
Created attachment 203109 [details] [review] opus: include streams count in caps
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().
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 ?
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.
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 ?
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.
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...
Created attachment 203160 [details] [review] opusenc: add upstream negotiation for elements that can't deal with multistream Such as the RTP payloader.
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].
Created attachment 203162 [details] [review] opusenc: add upstream negotiation for elements that can't deal with multistream Such as the RTP payloader.
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.
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