diff mbox

[FFmpeg-devel,PATCHv3] avformat/mpegts: opus muxing & demuxing for mapping family 255

Message ID b7aed1af-74ce-b3f1-595b-988f5325e2ce@gmail.com
State New
Headers show

Commit Message

pkv.stream Sept. 14, 2017, 4:48 p.m. UTC 
Thanks for your comments Moritz.
Corrected patch in attachment.
Regards

Le 14/09/2017 à 5:46 PM, Moritz Barsnick a écrit :
> On Fri, Sep 08, 2017 at 01:46:38 +0200, pkv.stream wrote:
>> -                    avpriv_request_sample(fc, "Opus in MPEG-TS - channel_config_code > 0x8");
> [...]
>> +                            avpriv_request_sample(fc, "Opus in MPEG-TS - channel_config_code");
> You probably need to mention the channel_config_code in the new
> message.
>
>> +                        for (j = 0; j < channels; j++) {
>> +                            opus_channel_map255[j] = j;
>> +                            }
> Misplaced closing bracket.
>
> Moritz
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
From 9175cb61dabce42417199cc6d6272d8290c39a5c Mon Sep 17 00:00:00 2001
From: pkviet <pkv.stream@gmail.com>
Date: Fri, 8 Sep 2017 01:34:22 +0200
Subject: [PATCH] avformat/mpegts: opus muxing & demuxing expanded

Support for opus in mpegts demuxer was brought by commit
9cfa68c560bdec82d2d5ec079f9c5b0f9ca37af0 (Kieran Kunhya).
Support for opus in mpegts muxer was then added by commit
01509cdf9287b975eced1fd609a8201fbd1438e3 (S. Droge).
Later commit 37941878f193a2316c514bd5ba55bfe9d2dfdfcf by Michael Graczyk
added support of mapping_family encoder parameter which allows for up to
255 audio channels (for family 255).
While matroska muxer & demuxer readily accepts mapping_family, it is not
the case for mpegts muxer & demuxer for all the range of the parameter
(family 255 and also part of family 1 with channel_config_code > 0x81
unsupported).
This commit brings such a support.
---
 libavformat/mpegts.c    | 64 ++++++++++++++++++++++++++++++++++++++++++++++---
 libavformat/mpegtsenc.c | 19 ++++++++++-----
 2 files changed, 74 insertions(+), 9 deletions(-)

Comments

pkv.stream Oct. 24, 2017, 9:27 p.m. UTC | #1 
Le 14/09/2017 à 6:48 PM, pkv.stream a écrit :
> Thanks for your comments Moritz.
> Corrected patch in attachment.
> Regards
>
> Le 14/09/2017 à 5:46 PM, Moritz Barsnick a écrit :
>> On Fri, Sep 08, 2017 at 01:46:38 +0200, pkv.stream wrote:
>>> - avpriv_request_sample(fc, "Opus in MPEG-TS - channel_config_code > 
>>> 0x8");
>> [...]
>>> + avpriv_request_sample(fc, "Opus in MPEG-TS - channel_config_code");
>> You probably need to mention the channel_config_code in the new
>> message.
>>
>>> +                        for (j = 0; j < channels; j++) {
>>> +                            opus_channel_map255[j] = j;
>>> +                            }
>> Misplaced closing bracket.
>>
>> Moritz
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
ping
Michael Niedermayer Oct. 25, 2017, 1:45 a.m. UTC | #2 
On Thu, Sep 14, 2017 at 06:48:33PM +0200, pkv.stream wrote:
> Thanks for your comments Moritz.
> Corrected patch in attachment.
> Regards
> 
> Le 14/09/2017 à 5:46 PM, Moritz Barsnick a écrit :
> >On Fri, Sep 08, 2017 at 01:46:38 +0200, pkv.stream wrote:
> >>-                    avpriv_request_sample(fc, "Opus in MPEG-TS - channel_config_code > 0x8");
> >[...]
> >>+                            avpriv_request_sample(fc, "Opus in MPEG-TS - channel_config_code");
> >You probably need to mention the channel_config_code in the new
> >message.
> >
> >>+                        for (j = 0; j < channels; j++) {
> >>+                            opus_channel_map255[j] = j;
> >>+                            }
> >Misplaced closing bracket.
> >
> >Moritz
> >_______________________________________________
> >ffmpeg-devel mailing list
> >ffmpeg-devel@ffmpeg.org
> >http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> 

>  mpegts.c    |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  mpegtsenc.c |   19 ++++++++++++-----
>  2 files changed, 74 insertions(+), 9 deletions(-)
> d2e34a72c7152584ae9423174f254d5d761325c3  0001-avformat-mpegts-opus-muxing-demuxing-expanded.patch
> From 9175cb61dabce42417199cc6d6272d8290c39a5c Mon Sep 17 00:00:00 2001
> From: pkviet <pkv.stream@gmail.com>
> Date: Fri, 8 Sep 2017 01:34:22 +0200
> Subject: [PATCH] avformat/mpegts: opus muxing & demuxing expanded
> 
> Support for opus in mpegts demuxer was brought by commit
> 9cfa68c560bdec82d2d5ec079f9c5b0f9ca37af0 (Kieran Kunhya).
> Support for opus in mpegts muxer was then added by commit
> 01509cdf9287b975eced1fd609a8201fbd1438e3 (S. Droge).
> Later commit 37941878f193a2316c514bd5ba55bfe9d2dfdfcf by Michael Graczyk
> added support of mapping_family encoder parameter which allows for up to
> 255 audio channels (for family 255).
> While matroska muxer & demuxer readily accepts mapping_family, it is not
> the case for mpegts muxer & demuxer for all the range of the parameter
> (family 255 and also part of family 1 with channel_config_code > 0x81
> unsupported).
> This commit brings such a support.
> ---
>  libavformat/mpegts.c    | 64 ++++++++++++++++++++++++++++++++++++++++++++++---
>  libavformat/mpegtsenc.c | 19 ++++++++++-----
>  2 files changed, 74 insertions(+), 9 deletions(-)
> 
> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> index 4d2f5c6..8d8977b 100644
> --- a/libavformat/mpegts.c
> +++ b/libavformat/mpegts.c
> @@ -1633,7 +1633,7 @@ static const uint8_t opus_stream_cnt[9] = {
>      1, 1, 1, 2, 2, 3, 4, 4, 5,
>  };
>  
> -static const uint8_t opus_channel_map[8][8] = {
> +static const uint8_t opus_channel_map_a[8][8] = {
>      { 0 },
>      { 0,1 },
>      { 0,2,1 },
> @@ -1644,6 +1644,17 @@ static const uint8_t opus_channel_map[8][8] = {
>      { 0,6,1,2,3,4,5,7 },
>  };
>  
> +static const uint8_t opus_channel_map_b[8][8] = {
> +    { 0 },
> +    { 0,1 },
> +    { 0,1,2 },
> +    { 0,1,2,3 },
> +    { 0,1,2,3,4 },
> +    { 0,1,2,3,4,5 },
> +    { 0,1,2,3,4,5,6 },
> +    { 0,1,2,3,4,5,6,7 },
> +};
> +
>  int ff_parse_mpeg2_descriptor(AVFormatContext *fc, AVStream *st, int stream_type,
>                                const uint8_t **pp, const uint8_t *desc_list_end,
>                                Mp4Descr *mp4_descr, int mp4_descr_count, int pid,
> @@ -1887,9 +1898,56 @@ int ff_parse_mpeg2_descriptor(AVFormatContext *fc, AVStream *st, int stream_type
>                      st->codecpar->extradata[18] = channel_config_code ? (channels > 2) : /* Dual Mono */ 255;
>                      st->codecpar->extradata[19] = opus_stream_cnt[channel_config_code];
>                      st->codecpar->extradata[20] = opus_coupled_stream_cnt[channel_config_code];
> -                    memcpy(&st->codecpar->extradata[21], opus_channel_map[channels - 1], channels);
> +                    memcpy(&st->codecpar->extradata[21], opus_channel_map_a[channels - 1], channels);
>                  } else {
> -                    avpriv_request_sample(fc, "Opus in MPEG-TS - channel_config_code > 0x8");
> +                    if (channel_config_code == 0x81) {
> +                        channels = get8(pp, desc_end);
> +                        st->codecpar->extradata_size = 22 + channels;
> +                        size_t extradata_size;
> +                        extradata_size = (22 + channels) * sizeof(uint8_t);
> +                        uint8_t *extradata;

this produces warnings:
libavformat/mpegts.c:1906:25: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]


> +                        extradata = av_malloc(extradata_size);
> +                        if (!extradata)
> +                            return AVERROR(ENOMEM);

> +                        for (i = 0; i <= (22+channels); i++) {

the extradata_size expression is repeated 3 times, code duplication
should be avoided


> +                            if (i < 9) {
> +                                extradata[i] = opus_default_extradata[i];
> +                            }
> +                            else {
> +                                extradata[i] = 0;
> +                            }
> +                        }

> +                        memcpy(st->codecpar->extradata, extradata, sizeof(extradata));

this looks wrong


> +                        av_free(extradata);
> +                        st->codecpar->extradata[9] = channels;
> +                        st->codecpar->extradata[18] = 255;
> +                        st->codecpar->extradata[19] = channels;
> +                        st->codecpar->extradata[20] = 0;
> +                        size_t channel_map_size = channels * sizeof(uint8_t);
> +                        uint8_t *opus_channel_map255;
> +                        opus_channel_map255 = av_malloc(channel_map_size);
> +                        if (!opus_channel_map255)
> +                            return AVERROR(ENOMEM);
> +                        uint8_t j;
> +                        for (j = 0; j < channels; j++) {
> +                            opus_channel_map255[j] = j;
> +                        }
> +                        memcpy(&st->codecpar->extradata[21], opus_channel_map255, channels);
> +                        av_free(opus_channel_map255);
> +                    } else {
> +                        if ((channel_config_code >= 0x82) && (channel_config_code <= 0x88)) {

> +                            channels = get8(pp, desc_end);
> +                            st->codecpar->extradata[9] = channels;
> +                            st->codecpar->extradata[18] = 1;
> +                            st->codecpar->extradata[19] = channels;
> +                            st->codecpar->extradata[20] = 0;
> +                            memcpy(&st->codecpar->extradata[21], opus_channel_map_b[channels - 1], channels);

the channels value is not checked before use in memcpy


> +                        }
> +                        else {
> +                            avpriv_request_sample(fc, "Opus in MPEG-TS - channel_config_code %i", channel_config_code);
> +                        }
> +
> +                    }
>                  }
>                  st->need_parsing = AVSTREAM_PARSE_FULL;
>                  st->internal->need_context_update = 1;
> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
> index fdfa544..af1dfc6 100644
> --- a/libavformat/mpegtsenc.c
> +++ b/libavformat/mpegtsenc.c

muxer and demuxer changes should be in seperate patches

thx

[...]
pkv.stream Oct. 28, 2017, 1:49 a.m. UTC | #3 
>>   int ff_parse_mpeg2_descriptor(AVFormatContext *fc, AVStream *st, int stream_type,
>>                                 const uint8_t **pp, const uint8_t *desc_list_end,
>>                                 Mp4Descr *mp4_descr, int mp4_descr_count, int pid,
>> @@ -1887,9 +1898,56 @@ int ff_parse_mpeg2_descriptor(AVFormatContext *fc, AVStream *st, int stream_type
>>                       st->codecpar->extradata[18] = channel_config_code ? (channels > 2) : /* Dual Mono */ 255;
>>                       st->codecpar->extradata[19] = opus_stream_cnt[channel_config_code];
>>                       st->codecpar->extradata[20] = opus_coupled_stream_cnt[channel_config_code];
>> -                    memcpy(&st->codecpar->extradata[21], opus_channel_map[channels - 1], channels);
>> +                    memcpy(&st->codecpar->extradata[21], opus_channel_map_a[channels - 1], channels);
>>                   } else {
>> -                    avpriv_request_sample(fc, "Opus in MPEG-TS - channel_config_code > 0x8");
>> +                    if (channel_config_code == 0x81) {
>> +                        channels = get8(pp, desc_end);
>> +                        st->codecpar->extradata_size = 22 + channels;
>> +                        size_t extradata_size;
>> +                        extradata_size = (22 + channels) * sizeof(uint8_t);
>> +                        uint8_t *extradata;
> this produces warnings:
> libavformat/mpegts.c:1906:25: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>

Thanks a lot Michael for your review.
I have cleaned this warning and others issued by gcc.

>> +                        extradata = av_malloc(extradata_size);
>> +                        if (!extradata)
>> +                            return AVERROR(ENOMEM);
>> +                        for (i = 0; i <= (22+channels); i++) {
> the extradata_size expression is repeated 3 times, code duplication
> should be avoided
>
removed
>> +                            if (i < 9) {
>> +                                extradata[i] = opus_default_extradata[i];
>> +                            }
>> +                            else {
>> +                                extradata[i] = 0;
>> +                            }
>> +                        }
>> +                        memcpy(st->codecpar->extradata, extradata, sizeof(extradata));
> this looks wrong
>
right, fixed (removed)

>> +                        av_free(extradata);
>> +                        st->codecpar->extradata[9] = channels;
>> +                        st->codecpar->extradata[18] = 255;
>> +                        st->codecpar->extradata[19] = channels;
>> +                        st->codecpar->extradata[20] = 0;
>> +                        size_t channel_map_size = channels * sizeof(uint8_t);
>> +                        uint8_t *opus_channel_map255;
>> +                        opus_channel_map255 = av_malloc(channel_map_size);
>> +                        if (!opus_channel_map255)
>> +                            return AVERROR(ENOMEM);
>> +                        uint8_t j;
>> +                        for (j = 0; j < channels; j++) {
>> +                            opus_channel_map255[j] = j;
>> +                        }
>> +                        memcpy(&st->codecpar->extradata[21], opus_channel_map255, channels);
>> +                        av_free(opus_channel_map255);
>> +                    } else {
>> +                        if ((channel_config_code >= 0x82) && (channel_config_code <= 0x88)) {
>> +                            channels = get8(pp, desc_end);
>> +                            st->codecpar->extradata[9] = channels;
>> +                            st->codecpar->extradata[18] = 1;
>> +                            st->codecpar->extradata[19] = channels;
>> +                            st->codecpar->extradata[20] = 0;
>> +                            memcpy(&st->codecpar->extradata[21], opus_channel_map_b[channels - 1], channels);
> the channels value is not checked before use in memcpy

fixed;
I have added other checks for all the data read from header;
>
>> +                        }
>> +                        else {
>> +                            avpriv_request_sample(fc, "Opus in MPEG-TS - channel_config_code %i", channel_config_code);
>> +                        }
>> +
>> +                    }
>>                   }
>>                   st->need_parsing = AVSTREAM_PARSE_FULL;
>>                   st->internal->need_context_update = 1;
>> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
>> index fdfa544..af1dfc6 100644
>> --- a/libavformat/mpegtsenc.c
>> +++ b/libavformat/mpegtsenc.c
> muxer and demuxer changes should be in seperate patches

done; see next posts.

My previous code was not following strictly the draft spec here:
https://people.xiph.org/~tterribe/opus/ETSI_TS_opus-v0.1.3-draft.doc
So I have rewritten it to have strict adherence regarding the bits 
allocated in header.

I have also run fate + patcheck ; no issues (just some unused variables 
warnings for variables antedating this patch).

Regards
diff mbox

Patch

diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
index 4d2f5c6..8d8977b 100644
--- a/libavformat/mpegts.c
+++ b/libavformat/mpegts.c
@@ -1633,7 +1633,7 @@  static const uint8_t opus_stream_cnt[9] = {
     1, 1, 1, 2, 2, 3, 4, 4, 5,
 };
 
-static const uint8_t opus_channel_map[8][8] = {
+static const uint8_t opus_channel_map_a[8][8] = {
     { 0 },
     { 0,1 },
     { 0,2,1 },
@@ -1644,6 +1644,17 @@  static const uint8_t opus_channel_map[8][8] = {
     { 0,6,1,2,3,4,5,7 },
 };
 
+static const uint8_t opus_channel_map_b[8][8] = {
+    { 0 },
+    { 0,1 },
+    { 0,1,2 },
+    { 0,1,2,3 },
+    { 0,1,2,3,4 },
+    { 0,1,2,3,4,5 },
+    { 0,1,2,3,4,5,6 },
+    { 0,1,2,3,4,5,6,7 },
+};
+
 int ff_parse_mpeg2_descriptor(AVFormatContext *fc, AVStream *st, int stream_type,
                               const uint8_t **pp, const uint8_t *desc_list_end,
                               Mp4Descr *mp4_descr, int mp4_descr_count, int pid,
@@ -1887,9 +1898,56 @@  int ff_parse_mpeg2_descriptor(AVFormatContext *fc, AVStream *st, int stream_type
                     st->codecpar->extradata[18] = channel_config_code ? (channels > 2) : /* Dual Mono */ 255;
                     st->codecpar->extradata[19] = opus_stream_cnt[channel_config_code];
                     st->codecpar->extradata[20] = opus_coupled_stream_cnt[channel_config_code];
-                    memcpy(&st->codecpar->extradata[21], opus_channel_map[channels - 1], channels);
+                    memcpy(&st->codecpar->extradata[21], opus_channel_map_a[channels - 1], channels);
                 } else {
-                    avpriv_request_sample(fc, "Opus in MPEG-TS - channel_config_code > 0x8");
+                    if (channel_config_code == 0x81) {
+                        channels = get8(pp, desc_end);
+                        st->codecpar->extradata_size = 22 + channels;
+                        size_t extradata_size;
+                        extradata_size = (22 + channels) * sizeof(uint8_t);
+                        uint8_t *extradata;
+                        extradata = av_malloc(extradata_size);
+                        if (!extradata)
+                            return AVERROR(ENOMEM);
+                        for (i = 0; i <= (22+channels); i++) {
+                            if (i < 9) {
+                                extradata[i] = opus_default_extradata[i];
+                            }
+                            else {
+                                extradata[i] = 0;
+                            }
+                        }
+                        memcpy(st->codecpar->extradata, extradata, sizeof(extradata));
+                        av_free(extradata);
+                        st->codecpar->extradata[9] = channels;
+                        st->codecpar->extradata[18] = 255;
+                        st->codecpar->extradata[19] = channels;
+                        st->codecpar->extradata[20] = 0;
+                        size_t channel_map_size = channels * sizeof(uint8_t);
+                        uint8_t *opus_channel_map255;
+                        opus_channel_map255 = av_malloc(channel_map_size);
+                        if (!opus_channel_map255)
+                            return AVERROR(ENOMEM);
+                        uint8_t j;
+                        for (j = 0; j < channels; j++) {
+                            opus_channel_map255[j] = j;
+                        }
+                        memcpy(&st->codecpar->extradata[21], opus_channel_map255, channels);
+                        av_free(opus_channel_map255);
+                    } else {
+                        if ((channel_config_code >= 0x82) && (channel_config_code <= 0x88)) {
+                            channels = get8(pp, desc_end);
+                            st->codecpar->extradata[9] = channels;
+                            st->codecpar->extradata[18] = 1;
+                            st->codecpar->extradata[19] = channels;
+                            st->codecpar->extradata[20] = 0;
+                            memcpy(&st->codecpar->extradata[21], opus_channel_map_b[channels - 1], channels);
+                        }
+                        else {
+                            avpriv_request_sample(fc, "Opus in MPEG-TS - channel_config_code %i", channel_config_code);
+                        }
+
+                    }
                 }
                 st->need_parsing = AVSTREAM_PARSE_FULL;
                 st->internal->need_context_update = 1;
diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
index fdfa544..af1dfc6 100644
--- a/libavformat/mpegtsenc.c
+++ b/libavformat/mpegtsenc.c
@@ -421,8 +421,8 @@  static int mpegts_write_pmt(AVFormatContext *s, MpegTSService *service)
                 *q++ = 'D';
             }
             if (st->codecpar->codec_id==AV_CODEC_ID_OPUS) {
-                /* 6 bytes registration descriptor, 4 bytes Opus audio descriptor */
-                if (q - data > SECTION_LENGTH - 6 - 4) {
+                /* 6 bytes registration descriptor, 6 bytes Opus audio descriptor */
+                if (q - data > SECTION_LENGTH - 6 - 6) {
                     err = 1;
                     break;
                 }
@@ -435,7 +435,7 @@  static int mpegts_write_pmt(AVFormatContext *s, MpegTSService *service)
                 *q++ = 's';
 
                 *q++ = 0x7f; /* DVB extension descriptor */
-                *q++ = 2;
+                *q++ = 4;
                 *q++ = 0x80;
 
                 if (st->codecpar->extradata && st->codecpar->extradata_size >= 19) {
@@ -483,9 +483,14 @@  static int mpegts_write_pmt(AVFormatContext *s, MpegTSService *service)
                             *q++ = 0xff;
                         }
                     } else {
-                        /* Unsupported */
-                        av_log(s, AV_LOG_ERROR, "Unsupported Opus channel mapping for family %d", st->codecpar->extradata[18]);
-                        *q++ = 0xff;
+                        /* mapping family 255 , set channel_config_code to 0x81 */
+                        if (st->codecpar->extradata[18] == 255) {
+                            *q++ = 0x81;
+                        } else {
+                            /* Unsupported */
+                            av_log(s, AV_LOG_ERROR, "Unsupported Opus channel mapping for family %d", st->codecpar->extradata[18]);
+                            *q++ = 0xff;
+                        }
                     }
                 } else if (st->codecpar->channels <= 2) {
                     /* Assume RTP mapping family */
@@ -495,6 +500,8 @@  static int mpegts_write_pmt(AVFormatContext *s, MpegTSService *service)
                     av_log(s, AV_LOG_ERROR, "Unsupported Opus channel mapping");
                     *q++ = 0xff;
                 }
+                *q++ = st->codecpar->extradata[9];
+                *q++ = st->codecpar->extradata[18];
             }
 
             if (lang) {