GNOME Bugzilla – Bug 757049
tsdemux: Add support for Opus
Last modified: 2015-11-03 18:44:14 UTC
See commit message
Created attachment 314010 [details] [review] tsdemux: Add support for Opus Code partially based on https://git.videolan.org/?p=ffmpeg.git;a=commit;h=74141f693ded2fbf75af56fff309d2db35183635 and based on the spec draft at https://wiki.xiph.org/OpusTS Makes it possible to demux http://www.obe.tv/Downloads/opus.ts
Created attachment 314011 [details] [review] tsdemux: Add support for Opus Code partially based on https://git.videolan.org/?p=ffmpeg.git;a=commit;h=74141f693ded2fbf75af56fff309d2db35183635 and based on the spec draft at https://wiki.xiph.org/OpusTS Makes it possible to demux http://www.obe.tv/Downloads/opus.ts
Comment on attachment 314011 [details] [review] tsdemux: Add support for Opus (Did not check against the draft spec, just commenting on the code) >+ packet_size = au_size - gst_byte_reader_get_pos (&reader); >+ if (!gst_byte_reader_dup_data (&reader, packet_size, &packet_data)) >+ goto error; I think an explicit check here that au_size is sane would be nicer, instead of guint underflow -> huge value -> dup_data() fails. >+ caps = gst_pad_get_current_caps (stream->pad); >+ s = gst_caps_get_structure (caps, 0); >+ if (!gst_structure_get (s, "streamheader", GST_TYPE_BUFFER, &streamheader, >+ NULL)) { >+ gst_caps_unref (caps); >+ goto error; >+ } >+ >+ /* FIXME: Do something with end trim */ >+ gst_buffer_map (streamheader, &map, GST_MAP_READ); >+ { >+ guint16 pre_skip = GST_READ_UINT16_BE (map.data + 10); >+ >+ if (pre_skip != start_trim) { >+ new_streamheader = gst_buffer_copy (streamheader); >+ start_trim = GUINT16_TO_BE (start_trim); >+ gst_buffer_fill (new_streamheader, 10, &start_trim, 2); >+ } >+ } >+ >+ gst_buffer_unmap (streamheader, &map); >+ gst_buffer_unref (streamheader); >+ if (new_streamheader) { >+ caps = gst_caps_make_writable (caps); >+ gst_caps_set_simple (caps, >+ "streamheader", GST_TYPE_BUFFER, new_streamheader, NULL); >+ gst_pad_set_caps (stream->pad, caps); >+ gst_buffer_unref (new_streamheader); >+ } >+ >+ gst_caps_unref (caps); >+ >+ return; >+... >+ } >+} >+ > static GstFlowReturn > gst_ts_demux_push_pending_data (GstTSDemux * demux, TSDemuxStream * stream) > { > ... > >+ if (bs->stream_type == GST_MPEGTS_STREAM_TYPE_PRIVATE_PES_PACKETS && >+ bs->registration_id == DRF_ID_OPUS) { >+ parse_opus_access_unit (stream); >+ } >+ This all looks a bit confusing to me. Are we doing caps/streamheader stuff for every single packet? Potentially changing the streamheader for every single packet?
(In reply to Tim-Philipp Müller from comment #3) > Comment on attachment 314011 [details] [review] [review] > tsdemux: Add support for Opus > > (Did not check against the draft spec, just commenting on the code) > > >+ packet_size = au_size - gst_byte_reader_get_pos (&reader); > >+ if (!gst_byte_reader_dup_data (&reader, packet_size, &packet_data)) > >+ goto error; > > I think an explicit check here that au_size is sane would be nicer, instead > of guint underflow -> huge value -> dup_data() fails. Sure, but the effect would be the same :) > This all looks a bit confusing to me. Are we doing caps/streamheader stuff > for every single packet? Potentially changing the streamheader for every > single packet? Yes, if every single packet has a different trim_start value. But actually it's not 100% correct, the spec says that it can only be at the beginning of a stream and applies to the current packet. Once a packet was there without the field, the field can't be used again. Also we need to handle trim_stop somehow. I think the best would be to introduce a GstMeta for this, however it would have to be used by tsdemux (and matroskademux and qtdemux) and opusdec/opusenc. So should probably go in libpbutils or libgstaudio?
> Sure, but the effect would be the same :) That's why I said "it would be nicer". I've had a quick look at the spec now, and think I understand what you're trying to do there with the streamheader stuff. Basically, streamheaders contain a value for trim_start as well, and if it's not consistent with the one in the packet header, then we fix up the streamheaders, ok. - the code retrieves caps + streamheader buffer + maps streamheader buffer for every single packet. That's not needed as far as I can tell, we should do that only in case trim_start is set and >0. Maybe move the fixing up into a separate function. - currently opusdec doesn't seem to handle the streamheader=(buffer)... case, only the array of buffers case > Yes, if every single packet has a different trim_start value. But actually > it's not 100% correct, the spec says that it can only be at the beginning of > a stream and applies to the current packet. Once a packet was there without > the field, the field can't be used again. Unless there was an end_trim I guess, for seamless concatenation? Not sure if we really need to do all this sanity/consistency checking at all. > Also we need to handle trim_stop somehow. I think the best would be to > introduce a GstMeta for this, however it would have to be used by tsdemux > (and matroskademux and qtdemux) and opusdec/opusenc. So should probably go > in libpbutils or libgstaudio? Were you thinking of an opus-specific meta? I was actually thinking of a more general coded-audio meta for this, which could be used for mp3/aac etc. as well then. I think we should just completely ignore the trim_start and trim_end stuff for now and add a FIXME, and sort it out in a more general way for all codecs, also see bug #746109 and especially bug #740196.
Ok, let's ignore trim_start/end for now then and fix it properly later. I'll update the patch with that, and your other suggestion later.
Created attachment 314032 [details] [review] tsdemux: Add support for Opus Code partially based on https://git.videolan.org/?p=ffmpeg.git;a=commit;h=74141f693ded2fbf75af56fff309d2db35183635 and based on the spec draft at https://wiki.xiph.org/OpusTS Makes it possible to demux http://www.obe.tv/Downloads/opus.ts
Created attachment 314033 [details] [review] opusdec: Add support for reading a single-buffer streamheader
(In reply to Tim-Philipp Müller from comment #5) > I've had a quick look at the spec now, and think I understand what you're > trying to do there with the streamheader stuff. Basically, streamheaders > contain a value for trim_start as well, and if it's not consistent with the > one in the packet header, then we fix up the streamheaders, ok. That's not what it does btw. Also we don't have the trim_start in the streamheader in mpegts (the descriptor only has the channel layout). The idea here is that each frame tells you how much of it has to be skipped at the beginning or end. So that if you would have to skip 1.5 frames, and you would start reading at the second frame, you would still know to skip 0.5 frames. Skipping is however only allowed at the beginning or end, not in the middle of the stream. So this is the same as what it does for the beginning in the Ogg header, just per packet.
> That's not what it does btw. Also we don't have the trim_start in the > streamheader in mpegts (the descriptor only has the channel layout). I think that's exactly what it does, it's just that you generate the header from a given fake opus header and just add the details like channel layout in addition, if needed. So we know the trim_start value is 0 at first (which I missed), unless trim_start is used multiple times, which it shouldn't be, no? > The idea here is that each frame tells you how much of it has to be skipped > at the beginning or end. So that if you would have to skip 1.5 frames, and > you would start reading at the second frame, you would still know to skip > 0.5 frames. > > Skipping is however only allowed at the beginning or end, not in the middle > of the stream. Of course. > So this is the same as what it does for the beginning in the Ogg header, > just per packet. I'm not sure where we're disagreeing/misunderstanding each other now tbh. I don't mind the updating of stream header per se, just that it was retrieved and mapped for every single packet. The fact that there might be multiple packets at the beginning that all have the trim_start set (legitimately) because more than one packet should be skipped indicates to me that this really should go into a meta and that we should not update the streamheader with this. Otherwise the decoder would have to differentiate between real format changes and just-updating-the-pre-skip-value, that sounds messy and not in line with how we do things usually (we don't keep state across renegotiation usually, do we).
(In reply to Tim-Philipp Müller from comment #10) > > That's not what it does btw. Also we don't have the trim_start in the > > streamheader in mpegts (the descriptor only has the channel layout). > > I think that's exactly what it does, it's just that you generate the header > from a given fake opus header and just add the details like channel layout > in addition, if needed. So we know the trim_start value is 0 at first (which > I missed), unless trim_start is used multiple times, which it shouldn't be, > no? In the streamheader it's for the complete stream, in the MPEGTS frame it's for this specific frame. That is, per frame the trim_start value can't be bigger than the duration of that frame, but you can have it on the following frame too if the complete first frame is to be skipped. That's the difference I wanted to point at :) > I don't mind the updating of stream header per se, just that it was > retrieved and mapped for every single packet. > > The fact that there might be multiple packets at the beginning that all have > the trim_start set (legitimately) because more than one packet should be > skipped indicates to me that this really should go into a meta and that we > should not update the streamheader with this. Otherwise the decoder would > have to differentiate between real format changes and > just-updating-the-pre-skip-value, that sounds messy and not in line with how > we do things usually (we don't keep state across renegotiation usually, do > we). I agree, this should be a generic skipping thing as a GstMeta and then GstAudioDecoder can just handle that. Or we could indeed do it with some segment tricks instead, but I would like to keep those out of the demuxers.
Created attachment 314076 [details] [review] tsdemux: Add support for Opus Code partially based on https://git.videolan.org/?p=ffmpeg.git;a=commit;h=74141f693ded2fbf75af56fff309d2db35183635 and based on the spec draft at https://wiki.xiph.org/OpusTS Makes it possible to demux http://www.obe.tv/Downloads/opus.ts
Created attachment 314077 [details] [review] opusdec: Add support for reading a single-buffer streamheader
Created attachment 314078 [details] [review] tsdemux: Add support for Opus channel mappings 0x82-0x88 and fix the two dual mono mappings
Created attachment 314079 [details] [review] mpegtsdescriptor: Add API for creating extended descriptors and fix writing them
Created attachment 314080 [details] [review] tsmux: Add support for Opus Not completely finished yet, the output is broken.
Created attachment 314081 [details] [review] tsdemux: Add support for Opus Code partially based on https://git.videolan.org/?p=ffmpeg.git;a=commit;h=74141f693ded2fbf75af56fff309d2db35183635 and based on the spec draft at https://wiki.xiph.org/OpusTS Makes it possible to demux http://www.obe.tv/Downloads/opus.ts
Created attachment 314082 [details] [review] opusdec: Add support for reading a single-buffer streamheader
Created attachment 314083 [details] [review] tsdemux: Add support for Opus channel mappings 0x82-0x88 and fix the two dual mono mappings
Created attachment 314084 [details] [review] mpegtsdescriptor: Add API for creating extended descriptors and fix writing them
Created attachment 314085 [details] [review] tsmux: Call prepare function for each collected buffer Not when clipping buffers, as that doesn't happen for every buffer.
Created attachment 314086 [details] [review] tsmux: Add support for Opus
Created attachment 314087 [details] [review] tsmux: Don't leak buffer in error cases
This all works now, also with ffmpeg. (In reply to Sebastian Dröge (slomo) from comment #18) > Created attachment 314082 [details] [review] [review] > opusdec: Add support for reading a single-buffer streamheader Maybe instead of this we should always make it an array and insert a dummy/empty comment buffer as second one?
Comment on attachment 314084 [details] [review] mpegtsdescriptor: Add API for creating extended descriptors and fix writing them The 'fix writing them' should probably be a separate commit? (N.B. I did not check if the fix is correct)
(In reply to Tim-Philipp Müller from comment #25) > Comment on attachment 314084 [details] [review] [review] > mpegtsdescriptor: Add API for creating extended descriptors and fix writing > them > > The 'fix writing them' should probably be a separate commit? (N.B. I did not > check if the fix is correct) I considered that first, but then thought it's too much noise. Especially when considering that this code was completely unused before.
Created attachment 314146 [details] [review] tsdemux: Add support for Opus Code partially based on https://git.videolan.org/?p=ffmpeg.git;a=commit;h=74141f693ded2fbf75af56fff309d2db35183635 and based on the spec draft at https://wiki.xiph.org/OpusTS Makes it possible to demux http://www.obe.tv/Downloads/opus.ts
Created attachment 314147 [details] [review] mpegtsdescriptor: Add API for creating extended descriptors and fix writing them
Created attachment 314148 [details] [review] tsmux: Call prepare function for each collected buffer Not when clipping buffers, as that doesn't happen for every buffer.
Created attachment 314149 [details] [review] tsmux: Add support for Opus
Created attachment 314150 [details] [review] tsmux: Don't leak buffer in error cases
Review of attachment 314149 [details] [review]: ::: gst/mpegtsmux/mpegtsmux.c @@ +708,3 @@ + opus_channel_config_code = map.data[9]; + } else if (map.data[9] >= 2 && map.data[9] <= 8 && map.data[18] == 1) { + /* Vorbis mapping */ Here we actually have to check if it's one of the two simple variants from the Opus/TS spec by looking at the stream count, coupled stream count and channel mapping array. And otherwise write the extended descriptor
Created attachment 314473 [details] [review] tsdemux: Add support for Opus Code partially based on https://git.videolan.org/?p=ffmpeg.git;a=commit;h=74141f693ded2fbf75af56fff309d2db35183635 and based on the spec draft at https://wiki.xiph.org/OpusTS Makes it possible to demux http://www.obe.tv/Downloads/opus.ts
Created attachment 314475 [details] [review] mpegtsdescriptor: Add API for creating extended descriptors and fix writing them
Created attachment 314476 [details] [review] tsmux: Call prepare function for each collected buffer Not when clipping buffers, as that doesn't happen for every buffer.
Created attachment 314477 [details] [review] tsmux: Add support for Opus
Created attachment 314478 [details] [review] tsmux: Don't leak buffer in error cases
Created attachment 314515 [details] [review] tsdemux: Add support for Opus Code partially based on https://git.videolan.org/?p=ffmpeg.git;a=commit;h=74141f693ded2fbf75af56fff309d2db35183635 and based on the spec draft at https://wiki.xiph.org/OpusTS Makes it possible to demux http://www.obe.tv/Downloads/opus.ts
Comment on attachment 314515 [details] [review] tsdemux: Add support for Opus Sorry, one more nitpick: >+static void >+parse_opus_access_unit (TSDemuxStream * stream) >+{ >+ ... >+ >+ if (stream->current_size < 2) { >+ goto error; >+ } >+ >+ id = GST_READ_UINT16_BE (stream->data); >+ /* No control header */ >+ if ((id >> 5) != 0x3ff) { >+ return; >+ } >+ >+ gst_byte_reader_init (&reader, stream->data, stream->current_size); >+ gst_byte_reader_skip_unchecked (&reader, 2); Any reason not to just init the byte reader first thing and then do if (!gst_byte_reader_get_uint16_be (...)) goto error; ?
Created attachment 314545 [details] [review] tsdemux: Add support for Opus Code partially based on https://git.videolan.org/?p=ffmpeg.git;a=commit;h=74141f693ded2fbf75af56fff309d2db35183635 and based on the spec draft at https://wiki.xiph.org/OpusTS Makes it possible to demux http://www.obe.tv/Downloads/opus.ts
Created attachment 314586 [details] [review] tsmux: Add support for Opus
Created attachment 314591 [details] [review] tsdemux: Add support for Opus Code partially based on https://git.videolan.org/?p=ffmpeg.git;a=commit;h=74141f693ded2fbf75af56fff309d2db35183635 and based on the spec draft at https://wiki.xiph.org/OpusTS Makes it possible to demux http://www.obe.tv/Downloads/opus.ts
Created attachment 314592 [details] [review] tsmux: Add support for Opus
Created attachment 314607 [details] [review] tsdemux: Add support for Opus Code partially based on https://git.videolan.org/?p=ffmpeg.git;a=commit;h=74141f693ded2fbf75af56fff309d2db35183635 and based on the spec draft at https://wiki.xiph.org/OpusTS Makes it possible to demux http://www.obe.tv/Downloads/opus.ts
Created attachment 314608 [details] [review] tsmux: Add support for Opus
Created attachment 314729 [details] [review] tsdemux: Add support for Opus Code partially based on https://git.videolan.org/?p=ffmpeg.git;a=commit;h=74141f693ded2fbf75af56fff309d2db35183635 and based on the spec draft at https://wiki.xiph.org/OpusTS Makes it possible to demux http://www.obe.tv/Downloads/opus.ts
Created attachment 314730 [details] [review] tsmux: Add support for Opus
All merged