Patch to libavc/opus to create extradata if missing
This patch to libavcodec/opus.c will create the opus extradata if it is missing. This is required when muxing opus data from an RTP demuxer. Without this patch, the opux muxer will fail with a "No extradata present" error. This issue was first reported by Juan Navarro here: http://www.ffmpeg-archive.org/Unable-to-record-VP8-Opus-from-RTP-SDP-td46890... This patch was first created by Carl Eugen Hoyos, but didn't land because resulting opus files wouldn't play in VLC. The reason is because the opus headers in the original patch were too large. This patch update's Carl's original patch to limit the size of the extradata to 19 bytes.
Jonathan Baudanza:
This patch to libavcodec/opus.c will create the opus extradata if it is missing. This is required when muxing opus data from an RTP demuxer. Without this patch, the opux muxer will fail with a "No extradata present" error.
This issue was first reported by Juan Navarro here: http://www.ffmpeg-archive.org/Unable-to-record-VP8-Opus-from-RTP-SDP-td46890...
This patch was first created by Carl Eugen Hoyos, but didn't land because resulting opus files wouldn't play in VLC. The reason is because the opus headers in the original patch were too large. This patch update's Carl's original patch to limit the size of the extradata to 19 bytes.
extradata is documented to be set/allocated by the user for decoders; yet you intend to allocate it for a decoder. This might leak if the user uses avcodec_close() (namely if the caller doesn't have any code to free the extradata, believing there would be none, because this field is documented not to be set by libavcodec when decoding). Maybe add Opus support to extract_extradata instead? - Andreas
On 12/28/2020 6:53 AM, Andreas Rheinhardt wrote:
Jonathan Baudanza:
This patch to libavcodec/opus.c will create the opus extradata if it is missing. This is required when muxing opus data from an RTP demuxer. Without this patch, the opux muxer will fail with a "No extradata present" error.
As Andreas said, decoders don't allocate extradata. The libavcodec user does. Also, sounds like your patch would do nothing for builds with decoders disabled.
This issue was first reported by Juan Navarro here: http://www.ffmpeg-archive.org/Unable-to-record-VP8-Opus-from-RTP-SDP-td46890...
This patch was first created by Carl Eugen Hoyos, but didn't land because resulting opus files wouldn't play in VLC. The reason is because the opus headers in the original patch were too large. This patch update's Carl's original patch to limit the size of the extradata to 19 bytes.
extradata is documented to be set/allocated by the user for decoders; yet you intend to allocate it for a decoder. This might leak if the user uses avcodec_close() (namely if the caller doesn't have any code to free the extradata, believing there would be none, because this field is documented not to be set by libavcodec when decoding). Maybe add Opus support to extract_extradata instead?
An extract_extradata implementation requires the extradata to be in-band, and that doesn't seem to be the case with Opus. No demuxer exports a packet with the OpusHeader. It's in CodecPrivate in Matroska/Webm, and in a header OggPacket in Ogg that's directly exported as extradata. The RTP demuxer should start exporting extradata instead, if possible. Either generated from available stream information, or taken verbatim if present. Not sure how the RTP encapsulation for Opus is defined in this regard.
- Andreas _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Dec 28, 2020, 14:34 by jamrial@gmail.com:
On 12/28/2020 6:53 AM, Andreas Rheinhardt wrote:
Jonathan Baudanza:
This patch to libavcodec/opus.c will create the opus extradata if it is missing. This is required when muxing opus data from an RTP demuxer. Without this patch, the opux muxer will fail with a "No extradata present" error.
As Andreas said, decoders don't allocate extradata. The libavcodec user does. Also, sounds like your patch would do nothing for builds with decoders disabled.
This issue was first reported by Juan Navarro here: http://www.ffmpeg-archive.org/Unable-to-record-VP8-Opus-from-RTP-SDP-td46890...
This patch was first created by Carl Eugen Hoyos, but didn't land because resulting opus files wouldn't play in VLC. The reason is because the opus headers in the original patch were too large. This patch update's Carl's original patch to limit the size of the extradata to 19 bytes.
extradata is documented to be set/allocated by the user for decoders; yet you intend to allocate it for a decoder. This might leak if the user uses avcodec_close() (namely if the caller doesn't have any code to free the extradata, believing there would be none, because this field is documented not to be set by libavcodec when decoding). Maybe add Opus support to extract_extradata instead?
An extract_extradata implementation requires the extradata to be in-band, and that doesn't seem to be the case with Opus. No demuxer exports a packet with the OpusHeader. It's in CodecPrivate in Matroska/Webm, and in a header OggPacket in Ogg that's directly exported as extradata.
Actually its not impossible to create extradata from raw Opus streams. The extradata only contains a magic, #channes, padding, a sample rate and a channel_family. We know the magic, the #channels can be found out via the packet's contents and whether it contains multiple packets, the padding would be 0 because its a stream, the sample rate is always 48000 (we purpusefully disrespect the container's sample rate in case of Opus, which this is), and the mapping family can be set to 255 to just let the decoder decide on the number of channels.
The RTP demuxer should start exporting extradata instead, if possible. Either generated from available stream information, or taken verbatim if present. Not sure how the RTP encapsulation for Opus is defined in this regard.
Yes, I think that's the best solution. Just follow the steps I described above, and to write it out you can check out the opus_write_extradata() function in libavcodec/opusenc.c.
Actually its not impossible to create extradata from raw Opus streams. The extradata only contains a magic, #channes, padding, a sample rate and a channel_family.
We know the magic, the #channels can be found out via the packet's contents and whether it contains multiple packets, the padding would be 0 because its a stream, the sample rate is always 48000 (we purpusefully disrespect the container's sample rate in case of Opus, which this is), and the mapping family can be set to 255 to just let the decoder decide on the number of channels.
The RTP demuxer should start exporting extradata instead, if possible. Either generated from available stream information, or taken verbatim if present. Not sure how the RTP encapsulation for Opus is defined in this regard.
Yes, I think that's the best solution. Just follow the steps I described above, and to write it out you can check out the opus_write_extradata() function in libavcodec/opusenc.c.
Thank you for the feedback everyone. I agree, the RTP demuxer is the best place for this. I will resubmit the patch as such. I have a question about "purposefully disrespecting the container sample rate". In the case of SDP, it is possible to include the sample rate of the source in the sprop-maxcapturerate parameter in the a=fmtp section. For example (Taken from RFC7587 Section 7): m=audio 54312 RTP/AVP 101 a=rtpmap:101 opus/48000/2 a=fmtp:101 maxplaybackrate=16000; sprop-maxcapturerate=16000; maxaveragebitrate=20000; stereo=1; useinbandfec=1; usedtx=0 a=ptime:40 a=maxptime:40 In this case, would it be appropriate to set the sample rate in the opus header to 16000? I think in any case, we are allowed to set it to 0, but if the source sample rate is available we might as well use it.
Dec 28, 2020, 19:03 by jon@jonb.org:
Actually its not impossible to create extradata from raw Opus streams. The extradata only contains a magic, #channes, padding, a sample rate and a channel_family.
We know the magic, the #channels can be found out via the packet's contents and whether it contains multiple packets, the padding would be 0 because its a stream, the sample rate is always 48000 (we purpusefully disrespect the container's sample rate in case of Opus, which this is), and the mapping family can be set to 255 to just let the decoder decide on the number of channels.
The RTP demuxer should start exporting extradata instead, if possible. Either generated from available stream information, or taken verbatim if present. Not sure how the RTP encapsulation for Opus is defined in this regard.
Yes, I think that's the best solution. Just follow the steps I described above, and to write it out you can check out the opus_write_extradata() function in libavcodec/opusenc.c.
Thank you for the feedback everyone. I agree, the RTP demuxer is the best place for this. I will resubmit the patch as such.
I have a question about "purposefully disrespecting the container sample rate". In the case of SDP, it is possible to include the sample rate of the source in the sprop-maxcapturerate parameter in the a=fmtp section. For example (Taken from RFC7587 Section 7):
m=audio 54312 RTP/AVP 101 a=rtpmap:101 opus/48000/2 a=fmtp:101 maxplaybackrate=16000; sprop-maxcapturerate=16000; maxaveragebitrate=20000; stereo=1; useinbandfec=1; usedtx=0 a=ptime:40 a=maxptime:40
In this case, would it be appropriate to set the sample rate in the opus header to 16000? I think in any case, we are allowed to set it to 0, but if the source sample rate is available we might as well use it.
No, not really. The decoder doesn't even bother reading the samplerate value, because Opus is only 48Khz and we never convert anything in decoders. Xiph's opusdec tool does and resamples down to the 'original' samplerate, because they're wrong, and want to confuse people to hide away the "Opus doesn't support 44.1Khz" fact as much as possible, which is sort of understandable.
On Tue, Dec 29, 2020, at 6:41 AM, Lynne wrote:
Dec 28, 2020, 19:03 by jon@jonb.org:
Actually its not impossible to create extradata from raw Opus streams. The extradata only contains a magic, #channes, padding, a sample rate and a channel_family.
We know the magic, the #channels can be found out via the packet's contents and whether it contains multiple packets, the padding would be 0 because its a stream, the sample rate is always 48000 (we purpusefully disrespect the container's sample rate in case of Opus, which this is), and the mapping family can be set to 255 to just let the decoder decide on the number of channels.
The RTP demuxer should start exporting extradata instead, if possible. Either generated from available stream information, or taken verbatim if present. Not sure how the RTP encapsulation for Opus is defined in this regard.
Yes, I think that's the best solution. Just follow the steps I described above, and to write it out you can check out the opus_write_extradata() function in libavcodec/opusenc.c.
Thank you for the feedback everyone. I agree, the RTP demuxer is the best place for this. I will resubmit the patch as such.
Thank you for all the feedback everyone. I've updated the patch to build the opus header inside of ff_rtp_parse_open in libavformat/rtpdec.c. I set the mapping_family to 0, since I believe Opus/RTP only supports 2 channels. I set the input sample rate to 0, which the spec considers "unspecified". Let me know what you think of this approach for these values. Happy to switch them to 255/48000 if you prefer. I copied the bytestream code from opusenc.c. There's now 3 or 4 different places in the codebase where opus headers are getting built. Let me know if you'd like me to refactor this a bit. I didn't want to touch too many components with my first patch.
On Tue, Dec 29, 2020, at 3:25 PM, Jonathan Baudanza wrote:
Thank you for all the feedback everyone. I've updated the patch to build the opus header inside of ff_rtp_parse_open in libavformat/rtpdec.c.
I set the mapping_family to 0, since I believe Opus/RTP only supports 2 channels. I set the input sample rate to 0, which the spec considers "unspecified". Let me know what you think of this approach for these values. Happy to switch them to 255/48000 if you prefer.
I copied the bytestream code from opusenc.c. There's now 3 or 4 different places in the codebase where opus headers are getting built. Let me know if you'd like me to refactor this a bit. I didn't want to touch too many components with my first patch.
I had a follow-up thought to this. Do you think it makes sense to check st->codecpar->extradata for NULL before assigning it? Some other component may have assigned extradata, although I'm not sure why it would. And in that case we don't want a memory leak.
On 12/30/2020 3:00 PM, Jonathan Baudanza wrote:
On Tue, Dec 29, 2020, at 3:25 PM, Jonathan Baudanza wrote:
Thank you for all the feedback everyone. I've updated the patch to build the opus header inside of ff_rtp_parse_open in libavformat/rtpdec.c.
I set the mapping_family to 0, since I believe Opus/RTP only supports 2 channels. I set the input sample rate to 0, which the spec considers "unspecified". Let me know what you think of this approach for these values. Happy to switch them to 255/48000 if you prefer.
I copied the bytestream code from opusenc.c. There's now 3 or 4 different places in the codebase where opus headers are getting built. Let me know if you'd like me to refactor this a bit. I didn't want to touch too many components with my first patch.
I had a follow-up thought to this. Do you think it makes sense to check st->codecpar->extradata for NULL before assigning it? Some other component may have assigned extradata, although I'm not sure why it would. And in that case we don't want a memory leak.
Nothing should have done it at that point. The demuxer allocates the AVStream after all. But to be sure you can do av_freep(&st->codecpar->extradata) to ensure it's NULL before anything else is done.
On Wed, Dec 30, 2020, at 10:34 AM, James Almer wrote:
Nothing should have done it at that point. The demuxer allocates the AVStream after all. But to be sure you can do av_freep(&st->codecpar->extradata) to ensure it's NULL before anything else is done.
Thanks James. I've updated the patch to include av_freep. If there's no other feedback, I think this is ready to go.
Jonathan Baudanza:
On Wed, Dec 30, 2020, at 10:34 AM, James Almer wrote:
Nothing should have done it at that point. The demuxer allocates the AVStream after all. But to be sure you can do av_freep(&st->codecpar->extradata) to ensure it's NULL before anything else is done.
Thanks James.
I've updated the patch to include av_freep. If there's no other feedback, I think this is ready to go.
It would be better if you used ff_alloc_extradata(): It also already frees the extradata that might be present. And it does only set the extradata_size after the allocation succeeded. But it is of course even better if you actually errored out in case of allocation failure. Furthermore, your patch will lead to "declaration after statement" (or something like that) warnings, because FFmpeg only allows variable declarations at the beginning of each block (this is a remnant from C90). - Andreas
On Sun, Jan 3, 2021, at 12:50 PM, Andreas Rheinhardt wrote:
It would be better if you used ff_alloc_extradata(): It also already frees the extradata that might be present. And it does only set the extradata_size after the allocation succeeded. But it is of course even better if you actually errored out in case of allocation failure.
Furthermore, your patch will lead to "declaration after statement" (or something like that) warnings, because FFmpeg only allows variable declarations at the beginning of each block (this is a remnant from C90).
Thanks Andreas! The patch now uses ff_alloc_extradata, and I moved the bitstream writer into its own function so it can create the bs variable on the stack. I wasn't sure of the best way to error out, since ff_rtp_parse_open doesn't return an error value. So I log the error and return NULL. Please advise if there is a better way.
Jan 3, 2021, 23:06 by jon@jonb.org:
On Sun, Jan 3, 2021, at 12:50 PM, Andreas Rheinhardt wrote:
It would be better if you used ff_alloc_extradata(): It also already frees the extradata that might be present. And it does only set the extradata_size after the allocation succeeded. But it is of course even better if you actually errored out in case of allocation failure.
Furthermore, your patch will lead to "declaration after statement" (or something like that) warnings, because FFmpeg only allows variable declarations at the beginning of each block (this is a remnant from C90).
Thanks Andreas!
The patch now uses ff_alloc_extradata, and I moved the bitstream writer into its own function so it can create the bs variable on the stack.
I wasn't sure of the best way to error out, since ff_rtp_parse_open doesn't return an error value. So I log the error and return NULL. Please advise if there is a better way.
+ /* Input sample rate (0 = unspecified) */> + bytestream_put_le32 (&bs, 0); Put in 48000 here. Stream copy will preserve extradata, and we don't want to generate weird streams, even if our decoder ignores this.
+ /* Channel count */> + bytestream_put_byte (&bs, codecpar->channels);> + /* Mapping family */> + bytestream_put_byte (&bs, 0x0); This will only work for mono and stereo. The decoder will error out if more than 2 channels are present. For now maybe error out if the number of channels is greater than 2?
On Sun, Jan 3, 2021, at 2:25 PM, Lynne wrote:
+ /* Input sample rate (0 = unspecified) */> + bytestream_put_le32 (&bs, 0); Put in 48000 here. Stream copy will preserve extradata, and we don't want to generate weird streams, even if our decoder ignores this.
+ /* Channel count */> + bytestream_put_byte (&bs, codecpar->channels);> + /* Mapping family */> + bytestream_put_byte (&bs, 0x0); This will only work for mono and stereo. The decoder will error out if more than 2 channels are present. For now maybe error out if the number of channels is greater than 2?
Ok! Sample rate changed to 48000. If codecpar->channels is greater than 2, AVERROR_INVALIDDATA will be returned. Please advise if there is a better error code. Thanks Lynne!
Jan 3, 2021, 23:48 by jon@jonb.org:
On Sun, Jan 3, 2021, at 2:25 PM, Lynne wrote:
+ /* Input sample rate (0 = unspecified) */> + bytestream_put_le32 (&bs, 0); Put in 48000 here. Stream copy will preserve extradata, and we don't want to generate weird streams, even if our decoder ignores this.
+ /* Channel count */> + bytestream_put_byte (&bs, codecpar->channels);> + /* Mapping family */> + bytestream_put_byte (&bs, 0x0); This will only work for mono and stereo. The decoder will error out if more than 2 channels are present. For now maybe error out if the number of channels is greater than 2?
Ok! Sample rate changed to 48000. If codecpar->channels is greater than 2, AVERROR_INVALIDDATA will be returned. Please advise if there is a better error code.
Thanks Lynne!
+static int opus_write_extradata(AVCodecParameters *codecpar) { Function opening brackets must be put by themselves on a new line.
+ if (codecpar->channels > 2) { + return AVERROR_INVALIDDATA; + } + + ret = ff_alloc_extradata(codecpar, 19); + if (ret < 0) { + return ret; + } We don't wrap 1-line conditions in brackets, so just remove them.
+ av_log(s1, AV_LOG_ERROR, + "Error creating opus extradata: %s\n", + av_err2str(ret)); Weird indentation. There's an extra space in the 2 lines.
Apart from that LGTM.
On 1/3/2021 8:05 PM, Lynne wrote:
Jan 3, 2021, 23:48 by jon@jonb.org:
On Sun, Jan 3, 2021, at 2:25 PM, Lynne wrote:
+ /* Input sample rate (0 = unspecified) */> + bytestream_put_le32 (&bs, 0); Put in 48000 here. Stream copy will preserve extradata, and we don't want to generate weird streams, even if our decoder ignores this.
+ /* Channel count */> + bytestream_put_byte (&bs, codecpar->channels);> + /* Mapping family */> + bytestream_put_byte (&bs, 0x0); This will only work for mono and stereo. The decoder will error out if more than 2 channels are present. For now maybe error out if the number of channels is greater than 2?
Ok! Sample rate changed to 48000. If codecpar->channels is greater than 2, AVERROR_INVALIDDATA will be returned. Please advise if there is a better error code.
Thanks Lynne!
+static int opus_write_extradata(AVCodecParameters *codecpar) { Function opening brackets must be put by themselves on a new line.
+ if (codecpar->channels > 2) { + return AVERROR_INVALIDDATA; + } + + ret = ff_alloc_extradata(codecpar, 19); + if (ret < 0) { + return ret; + } We don't wrap 1-line conditions in brackets, so just remove them.
+ av_log(s1, AV_LOG_ERROR, + "Error creating opus extradata: %s\n", + av_err2str(ret)); Weird indentation. There's an extra space in the 2 lines.
Apart from that LGTM.
Made those changes and pushed it.
On Sun, Jan 3, 2021, at 3:33 PM, James Almer wrote:
Made those changes and pushed it.
Thanks for your help everyone! Looking forward to deploying this!
Lynne:
Jan 3, 2021, 23:48 by jon@jonb.org:
On Sun, Jan 3, 2021, at 2:25 PM, Lynne wrote:
+ /* Input sample rate (0 = unspecified) */> + bytestream_put_le32 (&bs, 0); Put in 48000 here. Stream copy will preserve extradata, and we don't want to generate weird streams, even if our decoder ignores this.
+ /* Channel count */> + bytestream_put_byte (&bs, codecpar->channels);> + /* Mapping family */> + bytestream_put_byte (&bs, 0x0); This will only work for mono and stereo. The decoder will error out if more than 2 channels are present. For now maybe error out if the number of channels is greater than 2?
Ok! Sample rate changed to 48000. If codecpar->channels is greater than 2, AVERROR_INVALIDDATA will be returned. Please advise if there is a better error code.
Thanks Lynne!
+static int opus_write_extradata(AVCodecParameters *codecpar) { Function opening brackets must be put by themselves on a new line.
+ if (codecpar->channels > 2) { + return AVERROR_INVALIDDATA; + } + + ret = ff_alloc_extradata(codecpar, 19); + if (ret < 0) { + return ret; + } We don't wrap 1-line conditions in brackets, so just remove them.
+ av_log(s1, AV_LOG_ERROR, + "Error creating opus extradata: %s\n", + av_err2str(ret)); Weird indentation. There's an extra space in the 2 lines.
Apart from that LGTM.
+1 if the case of more than two channels has been properly tested. - Andreas
On Sun, Jan 3, 2021, at 3:34 PM, Andreas Rheinhardt wrote:
Lynne:
Apart from that LGTM.
+1 if the case of more than two channels has been properly tested.
I tested this by creating an (invalid) SDP file with channels set to 3. In this case, the rtp demuxer fails with the following message: [sdp @ 0x7fe40280b800] Error creating opus extradata: Invalid data found when processing input It might be more descriptive if we added a log warning about the channel count. WDYT?
Jonathan Baudanza:
On Sun, Jan 3, 2021, at 3:34 PM, Andreas Rheinhardt wrote:
Lynne:
Apart from that LGTM.
+1 if the case of more than two channels has been properly tested.
I tested this by creating an (invalid) SDP file with channels set to 3. In this case, the rtp demuxer fails with the following message:
[sdp @ 0x7fe40280b800] Error creating opus extradata: Invalid data found when processing input
It might be more descriptive if we added a log warning about the channel count. WDYT?
It's ok as-is. - Andreas
Hi, On Sun, Jan 3, 2021 at 8:09 PM Andreas Rheinhardt < andreas.rheinhardt@gmail.com> wrote:
Jonathan Baudanza:
On Sun, Jan 3, 2021, at 3:34 PM, Andreas Rheinhardt wrote:
Lynne:
Apart from that LGTM.
+1 if the case of more than two channels has been properly tested.
I tested this by creating an (invalid) SDP file with channels set to 3. In this case, the rtp demuxer fails with the following message:
[sdp @ 0x7fe40280b800] Error creating opus extradata: Invalid data found when processing input
It might be more descriptive if we added a log warning about the channel count. WDYT?
It's ok as-is.
- Andreas
Could this land? I found another case that it fixes (specifically streamcopy + muxing a specific mkv file that was captured via RTP). Best, Tristan
On Wed, Jul 6, 2022 at 10:10 AM Tristan Matthews <tmatth@videolan.org> wrote:
Hi,
On Sun, Jan 3, 2021 at 8:09 PM Andreas Rheinhardt < andreas.rheinhardt@gmail.com> wrote:
Jonathan Baudanza:
On Sun, Jan 3, 2021, at 3:34 PM, Andreas Rheinhardt wrote:
Lynne:
Apart from that LGTM.
+1 if the case of more than two channels has been properly tested.
I tested this by creating an (invalid) SDP file with channels set to 3. In this case, the rtp demuxer fails with the following message:
[sdp @ 0x7fe40280b800] Error creating opus extradata: Invalid data found when processing input
It might be more descriptive if we added a log warning about the channel count. WDYT?
It's ok as-is.
- Andreas
Could this land? I found another case that it fixes (specifically streamcopy + muxing a specific mkv file that was captured via RTP).
Sorry for the noise, I missed in the thread that the code in question moved from the opus decoder to the RTP parser and landed there. Best, Tristan
participants (5)
-
Andreas Rheinhardt -
James Almer -
Jonathan Baudanza -
Lynne -
Tristan Matthews