diff mbox

[FFmpeg-devel] lavf/pcmdec: Map mime_type audio/L16 to the s16le demuxer

Message ID CAB0OVGpgHddmfD2HvykZvTET2R=tWw1D0z1CZoj=1BAn_2_ecg@mail.gmail.com
State Accepted
Headers show

Commit Message

Carl Eugen Hoyos Aug. 8, 2016, 10:51 p.m. UTC 
Hi!

2016-08-05 12:44 GMT+02:00 Nicolas George <george@nsup.org>:
> Le nonidi 19 thermidor, an CCXXIV, Carl Eugen Hoyos a écrit :
>> Hi!
>>
>> Attached patch implements RFC 2586.
>>
>> Please comment, Carl Eugen
>
>> From ba470c643c836826d75854e3e3539eb09ddd288a Mon Sep 17 00:00:00 2001
>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>> Date: Fri, 5 Aug 2016 12:22:17 +0200
>> Subject: [PATCH] lavf/pcmdec: Map mime_type audio/L16 to s16le as specified
>>  in RFC 2586.
>>
>> ---
>>  libavformat/pcmdec.c |   63 +++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 42 insertions(+), 21 deletions(-)
>>
>> diff --git a/libavformat/pcmdec.c b/libavformat/pcmdec.c
>> index df94345..36ef2c2 100644
>> --- a/libavformat/pcmdec.c
>> +++ b/libavformat/pcmdec.c
>> @@ -36,6 +36,7 @@ static int pcm_read_header(AVFormatContext *s)
>>  {
>>      PCMAudioDemuxerContext *s1 = s->priv_data;
>>      AVStream *st;
>> +    uint8_t *mime_type_opt = NULL;
>>
>>      st = avformat_new_stream(s, NULL);
>>      if (!st)
>> @@ -47,6 +48,25 @@ static int pcm_read_header(AVFormatContext *s)
>>      st->codecpar->sample_rate = s1->sample_rate;
>>      st->codecpar->channels    = s1->channels;
>>
>> +    av_opt_get(s->pb, "mime_type", AV_OPT_SEARCH_CHILDREN, &mime_type_opt);
>> +    if (mime_type_opt) {
>
>> +        const char *mime_type = mime_type_opt;
>
> I do not understand the need for that line.

Removed.

>> +        size_t len = strlen(s->iformat->mime_type);
>
> Maybe I am missing something obvious, but I think s->iformat->mime_type is
> NULL for all the other formats. It needs to be checked.

Added the check.

>> +        int rate, channels = 0;
>> +        if (!av_strncasecmp(s->iformat->mime_type, mime_type, len)) {
>
>> +            if (   !sscanf(mime_type + len, ";rate=%d;channels=%d", &rate, &channels)
>
> If I understand the way MIME type works, ";channels=2;rate=48000" would be
> exactly as valid, and spaces can surround the semicolons.

I changed this hunk.

>> +                || !rate) {
>> +                av_log(s, AV_LOG_ERROR,
>> +                       "Invalid sample_rate found in mime_type \"%s\"\n",
>> +                       mime_type);
>> +                return AVERROR_INVALIDDATA;
>> +            }
>> +            st->codecpar->sample_rate = rate;
>> +            if (channels)
>> +                st->codecpar->channels = channels;
>> +        }
>> +    }
>> +
>>      st->codecpar->bits_per_coded_sample =
>>          av_get_bits_per_sample(st->codecpar->codec_id);
>>
>> @@ -65,7 +85,7 @@ static const AVOption pcm_options[] = {
>>      { NULL },
>>  };
>>
>> -#define PCMDEF(name_, long_name_, ext, codec)               \
>
>> +#define PCMDEF(name_, long_name_, ext, codec, mime_type_)   \
>
> Instead of changing PCMDEF and all the subsequent declarations, you can
> create a new macro PCMDEF_WITH_MIME.
>
> Even simpler: make PCMDEF varadic, add __ARGS__ in the structure definition.
> Then, adding the MIME type is just a matter of adding ".mime_type = ..." in
> the macro call.

Yes, much simpler.

New patch attached.

Thank you, Carl Eugen

Comments

Michael Niedermayer Aug. 17, 2016, 3:32 p.m. UTC | #1 
On Tue, Aug 09, 2016 at 12:51:30AM +0200, Carl Eugen Hoyos wrote:
> Hi!
> 
> 2016-08-05 12:44 GMT+02:00 Nicolas George <george@nsup.org>:
> > Le nonidi 19 thermidor, an CCXXIV, Carl Eugen Hoyos a écrit :
> >> Hi!
> >>
> >> Attached patch implements RFC 2586.
> >>
> >> Please comment, Carl Eugen
> >
> >> From ba470c643c836826d75854e3e3539eb09ddd288a Mon Sep 17 00:00:00 2001
> >> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
> >> Date: Fri, 5 Aug 2016 12:22:17 +0200
> >> Subject: [PATCH] lavf/pcmdec: Map mime_type audio/L16 to s16le as specified
> >>  in RFC 2586.
> >>
> >> ---
> >>  libavformat/pcmdec.c |   63 +++++++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 42 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/libavformat/pcmdec.c b/libavformat/pcmdec.c
> >> index df94345..36ef2c2 100644
> >> --- a/libavformat/pcmdec.c
> >> +++ b/libavformat/pcmdec.c
> >> @@ -36,6 +36,7 @@ static int pcm_read_header(AVFormatContext *s)
> >>  {
> >>      PCMAudioDemuxerContext *s1 = s->priv_data;
> >>      AVStream *st;
> >> +    uint8_t *mime_type_opt = NULL;
> >>
> >>      st = avformat_new_stream(s, NULL);
> >>      if (!st)
> >> @@ -47,6 +48,25 @@ static int pcm_read_header(AVFormatContext *s)
> >>      st->codecpar->sample_rate = s1->sample_rate;
> >>      st->codecpar->channels    = s1->channels;
> >>
> >> +    av_opt_get(s->pb, "mime_type", AV_OPT_SEARCH_CHILDREN, &mime_type_opt);
> >> +    if (mime_type_opt) {
> >
> >> +        const char *mime_type = mime_type_opt;
> >
> > I do not understand the need for that line.
> 
> Removed.
> 
> >> +        size_t len = strlen(s->iformat->mime_type);
> >
> > Maybe I am missing something obvious, but I think s->iformat->mime_type is
> > NULL for all the other formats. It needs to be checked.
> 
> Added the check.
> 
> >> +        int rate, channels = 0;
> >> +        if (!av_strncasecmp(s->iformat->mime_type, mime_type, len)) {
> >
> >> +            if (   !sscanf(mime_type + len, ";rate=%d;channels=%d", &rate, &channels)
> >
> > If I understand the way MIME type works, ";channels=2;rate=48000" would be
> > exactly as valid, and spaces can surround the semicolons.
> 
> I changed this hunk.
> 
> >> +                || !rate) {
> >> +                av_log(s, AV_LOG_ERROR,
> >> +                       "Invalid sample_rate found in mime_type \"%s\"\n",
> >> +                       mime_type);
> >> +                return AVERROR_INVALIDDATA;
> >> +            }
> >> +            st->codecpar->sample_rate = rate;
> >> +            if (channels)
> >> +                st->codecpar->channels = channels;
> >> +        }
> >> +    }
> >> +
> >>      st->codecpar->bits_per_coded_sample =
> >>          av_get_bits_per_sample(st->codecpar->codec_id);
> >>
> >> @@ -65,7 +85,7 @@ static const AVOption pcm_options[] = {
> >>      { NULL },
> >>  };
> >>
> >> -#define PCMDEF(name_, long_name_, ext, codec)               \
> >
> >> +#define PCMDEF(name_, long_name_, ext, codec, mime_type_)   \
> >
> > Instead of changing PCMDEF and all the subsequent declarations, you can
> > create a new macro PCMDEF_WITH_MIME.
> >
> > Even simpler: make PCMDEF varadic, add __ARGS__ in the structure definition.
> > Then, adding the MIME type is just a matter of adding ".mime_type = ..." in
> > the macro call.
> 
> Yes, much simpler.
> 
> New patch attached.
> 
> Thank you, Carl Eugen

>  pcmdec.c  |   34 ++++++++++++++++++++++++++++++++--
>  version.h |    2 +-
>  2 files changed, 33 insertions(+), 3 deletions(-)
> 069eb6ba5db3434e3cc6abca155d8a712c54a8e3  0002-lavf-pcmdec-Map-mime_type-audio-L16-to-the-s16le-dem.patch
> From 3ef0951db492f1425a574aac71fe73f144c2d99a Mon Sep 17 00:00:00 2001
> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
> Date: Tue, 9 Aug 2016 00:46:57 +0200
> Subject: [PATCH 2/2] lavf/pcmdec: Map mime_type audio/L16 to the s16le
>  demuxer as specified in RFC 2586.
> 
> ---
>  libavformat/pcmdec.c  |   34 ++++++++++++++++++++++++++++++++--
>  libavformat/version.h |    2 +-
>  2 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/pcmdec.c b/libavformat/pcmdec.c
> index df94345..e3cc2ae 100644
> --- a/libavformat/pcmdec.c
> +++ b/libavformat/pcmdec.c
> @@ -36,6 +36,7 @@ static int pcm_read_header(AVFormatContext *s)
>  {
>      PCMAudioDemuxerContext *s1 = s->priv_data;
>      AVStream *st;
> +    uint8_t *mime_type = NULL;
>  
>      st = avformat_new_stream(s, NULL);
>      if (!st)
> @@ -47,6 +48,34 @@ static int pcm_read_header(AVFormatContext *s)
>      st->codecpar->sample_rate = s1->sample_rate;
>      st->codecpar->channels    = s1->channels;
>  
> +    av_opt_get(s->pb, "mime_type", AV_OPT_SEARCH_CHILDREN, &mime_type);
> +    if (mime_type && s->iformat->mime_type) {
> +        int rate = 0, channels = 0;
> +        size_t len = strlen(s->iformat->mime_type);
> +        if (!strncmp(s->iformat->mime_type, mime_type, len)) {
> +            uint8_t *options = mime_type + len;
> +            len = strlen(mime_type);
> +            while (options < mime_type + len) {
> +                options = strstr(options, ";");
> +                if (!options++)
> +                    break;
> +                if (!rate)
> +                    sscanf(options, " rate=%d",     &rate);
> +                if (!channels)
> +                    sscanf(options, " channels=%d", &channels);
> +            }

rate and channels probably should be subject to some sanity checks
like < 0
(unless its checked later)

no further comments from me

[...]
Carl Eugen Hoyos Aug. 17, 2016, 4:14 p.m. UTC | #2 
Hi!

2016-08-17 17:32 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
>> +    av_opt_get(s->pb, "mime_type", AV_OPT_SEARCH_CHILDREN, &mime_type);
>> +    if (mime_type && s->iformat->mime_type) {
>> +        int rate = 0, channels = 0;
>> +        size_t len = strlen(s->iformat->mime_type);
>> +        if (!strncmp(s->iformat->mime_type, mime_type, len)) {
>> +            uint8_t *options = mime_type + len;
>> +            len = strlen(mime_type);
>> +            while (options < mime_type + len) {
>> +                options = strstr(options, ";");
>> +                if (!options++)
>> +                    break;
>> +                if (!rate)
>> +                    sscanf(options, " rate=%d",     &rate);
>> +                if (!channels)
>> +                    sscanf(options, " channels=%d", &channels);
>> +            }
>
> rate and channels probably should be subject to some sanity checks
> like < 0

Sanity checks for <0 added, INT_MAX is also possible with the current
code.

Patch applied, Carl Eugen
James Almer Aug. 18, 2016, 4:47 a.m. UTC | #3 
On 8/17/2016 1:14 PM, Carl Eugen Hoyos wrote:
> Hi!
> 
> 2016-08-17 17:32 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
>>> +    av_opt_get(s->pb, "mime_type", AV_OPT_SEARCH_CHILDREN, &mime_type);
>>> +    if (mime_type && s->iformat->mime_type) {
>>> +        int rate = 0, channels = 0;
>>> +        size_t len = strlen(s->iformat->mime_type);
>>> +        if (!strncmp(s->iformat->mime_type, mime_type, len)) {
>>> +            uint8_t *options = mime_type + len;
>>> +            len = strlen(mime_type);
>>> +            while (options < mime_type + len) {
>>> +                options = strstr(options, ";");
>>> +                if (!options++)
>>> +                    break;
>>> +                if (!rate)
>>> +                    sscanf(options, " rate=%d",     &rate);
>>> +                if (!channels)
>>> +                    sscanf(options, " channels=%d", &channels);
>>> +            }
>>
>> rate and channels probably should be subject to some sanity checks
>> like < 0
> 
> Sanity checks for <0 added, INT_MAX is also possible with the current
> code.
> 
> Patch applied, Carl Eugen

This broke msvc
http://fate.ffmpeg.org/log.cgi?time=20160817230315&log=compile&slot=x86_64-msvc12-windows-native

Moving __VA_ARGS__ to the end of the macro may be enough to fix this,
but i can't test.
Carl Eugen Hoyos Aug. 19, 2016, 8:33 a.m. UTC | #4 
Hi James!

> Am 18.08.2016 um 06:47 schrieb James Almer <jamrial@gmail.com>:
> 
>> On 8/17/2016 1:14 PM, Carl Eugen Hoyos wrote:
>> Hi!
>> 
>> 2016-08-17 17:32 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
>>>> +    av_opt_get(s->pb, "mime_type", AV_OPT_SEARCH_CHILDREN, &mime_type);
>>>> +    if (mime_type && s->iformat->mime_type) {
>>>> +        int rate = 0, channels = 0;
>>>> +        size_t len = strlen(s->iformat->mime_type);
>>>> +        if (!strncmp(s->iformat->mime_type, mime_type, len)) {
>>>> +            uint8_t *options = mime_type + len;
>>>> +            len = strlen(mime_type);
>>>> +            while (options < mime_type + len) {
>>>> +                options = strstr(options, ";");
>>>> +                if (!options++)
>>>> +                    break;
>>>> +                if (!rate)
>>>> +                    sscanf(options, " rate=%d",     &rate);
>>>> +                if (!channels)
>>>> +                    sscanf(options, " channels=%d", &channels);
>>>> +            }
>>> 
>>> rate and channels probably should be subject to some sanity checks
>>> like < 0
>> 
>> Sanity checks for <0 added, INT_MAX is also possible with the current
>> code.
>> 
>> Patch applied, Carl Eugen
> 
> This broke msvc
> http://fate.ffmpeg.org/log.cgi?time=20160817230315&log=compile&slot=x86_64-msvc12-windows-native
> 
> Moving __VA_ARGS__ to the end of the macro may be enough to fix this,
> but i can't test.

I pushed this change yesterday and fate is happy again.
Thank you!

Carl Eugen
diff mbox

Patch

From 3ef0951db492f1425a574aac71fe73f144c2d99a Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <cehoyos@ag.or.at>
Date: Tue, 9 Aug 2016 00:46:57 +0200
Subject: [PATCH 2/2] lavf/pcmdec: Map mime_type audio/L16 to the s16le
 demuxer as specified in RFC 2586.

---
 libavformat/pcmdec.c  |   34 ++++++++++++++++++++++++++++++++--
 libavformat/version.h |    2 +-
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/libavformat/pcmdec.c b/libavformat/pcmdec.c
index df94345..e3cc2ae 100644
--- a/libavformat/pcmdec.c
+++ b/libavformat/pcmdec.c
@@ -36,6 +36,7 @@  static int pcm_read_header(AVFormatContext *s)
 {
     PCMAudioDemuxerContext *s1 = s->priv_data;
     AVStream *st;
+    uint8_t *mime_type = NULL;
 
     st = avformat_new_stream(s, NULL);
     if (!st)
@@ -47,6 +48,34 @@  static int pcm_read_header(AVFormatContext *s)
     st->codecpar->sample_rate = s1->sample_rate;
     st->codecpar->channels    = s1->channels;
 
+    av_opt_get(s->pb, "mime_type", AV_OPT_SEARCH_CHILDREN, &mime_type);
+    if (mime_type && s->iformat->mime_type) {
+        int rate = 0, channels = 0;
+        size_t len = strlen(s->iformat->mime_type);
+        if (!strncmp(s->iformat->mime_type, mime_type, len)) {
+            uint8_t *options = mime_type + len;
+            len = strlen(mime_type);
+            while (options < mime_type + len) {
+                options = strstr(options, ";");
+                if (!options++)
+                    break;
+                if (!rate)
+                    sscanf(options, " rate=%d",     &rate);
+                if (!channels)
+                    sscanf(options, " channels=%d", &channels);
+            }
+            if (!rate) {
+                av_log(s, AV_LOG_ERROR,
+                       "Invalid sample_rate found in mime_type \"%s\"\n",
+                       mime_type);
+                return AVERROR_INVALIDDATA;
+            }
+            st->codecpar->sample_rate = rate;
+            if (channels)
+                st->codecpar->channels = channels;
+        }
+    }
+
     st->codecpar->bits_per_coded_sample =
         av_get_bits_per_sample(st->codecpar->codec_id);
 
@@ -65,7 +94,7 @@  static const AVOption pcm_options[] = {
     { NULL },
 };
 
-#define PCMDEF(name_, long_name_, ext, codec)               \
+#define PCMDEF(name_, long_name_, ext, codec, ...)          \
 static const AVClass name_ ## _demuxer_class = {            \
     .class_name = #name_ " demuxer",                        \
     .item_name  = av_default_item_name,                     \
@@ -82,6 +111,7 @@  AVInputFormat ff_pcm_ ## name_ ## _demuxer = {              \
     .flags          = AVFMT_GENERIC_INDEX,                  \
     .extensions     = ext,                                  \
     .raw_codec_id   = codec,                                \
+    __VA_ARGS__                                             \
     .priv_class     = &name_ ## _demuxer_class,             \
 };
 
@@ -113,7 +143,7 @@  PCMDEF(s16be, "PCM signed 16-bit big-endian",
        AV_NE("sw", NULL), AV_CODEC_ID_PCM_S16BE)
 
 PCMDEF(s16le, "PCM signed 16-bit little-endian",
-       AV_NE(NULL, "sw"), AV_CODEC_ID_PCM_S16LE)
+       AV_NE(NULL, "sw"), AV_CODEC_ID_PCM_S16LE, .mime_type = "audio/L16",)
 
 PCMDEF(s8, "PCM signed 8-bit",
        "sb", AV_CODEC_ID_PCM_S8)
diff --git a/libavformat/version.h b/libavformat/version.h
index 6f47a2f..590902d 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -33,7 +33,7 @@ 
 // Also please add any ticket numbers that you belive might be affected here
 #define LIBAVFORMAT_VERSION_MAJOR  57
 #define LIBAVFORMAT_VERSION_MINOR  46
-#define LIBAVFORMAT_VERSION_MICRO 101
+#define LIBAVFORMAT_VERSION_MICRO 102
 
 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
                                                LIBAVFORMAT_VERSION_MINOR, \
-- 
1.7.10.4