diff mbox

[FFmpeg-devel] flvdec: set need_context_update when changing codec id

Message ID 6ebf692b-439a-ef0e-37b7-56a1390228b9@googlemail.com
State Accepted
Headers show

Commit Message

Andreas Cadhalpun Nov. 24, 2016, 12:37 a.m. UTC 
On 23.11.2016 03:26, Michael Niedermayer wrote:
> On Fri, Nov 04, 2016 at 10:28:20PM +0100, Andreas Cadhalpun wrote:
>> Otherwise the codec context and codecpar might disagree on the codec id,
>> triggering asserts in av_parser_parse2.
>>
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> ---
>>  libavformat/flvdec.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
>> index e53c345..4ba7fc8 100644
>> --- a/libavformat/flvdec.c
>> +++ b/libavformat/flvdec.c
>> @@ -289,7 +289,9 @@ static int flv_same_video_codec(AVCodecParameters *vpar, int flags)
>>  static int flv_set_video_codec(AVFormatContext *s, AVStream *vstream,
>>                                 int flv_codecid, int read)
>>  {
>> +    int ret = 0;
>>      AVCodecParameters *par = vstream->codecpar;
>> +    enum AVCodecID old_codec_id = vstream->codecpar->codec_id;
>>      switch (flv_codecid) {
>>      case FLV_CODECID_H263:
>>          par->codec_id = AV_CODEC_ID_FLV1;
>> @@ -317,20 +319,26 @@ static int flv_set_video_codec(AVFormatContext *s, AVStream *vstream,
>>              else
>>                  avio_skip(s->pb, 1);
>>          }
>> -        return 1;     // 1 byte body size adjustment for flv_read_packet()
>> +        ret = 1;     // 1 byte body size adjustment for flv_read_packet()
>> +        break;
>>      case FLV_CODECID_H264:
>>          par->codec_id = AV_CODEC_ID_H264;
>>          vstream->need_parsing = AVSTREAM_PARSE_HEADERS;
>> -        return 3;     // not 4, reading packet type will consume one byte
>> +        ret = 3;     // not 4, reading packet type will consume one byte
>> +        break;
>>      case FLV_CODECID_MPEG4:
>>          par->codec_id = AV_CODEC_ID_MPEG4;
>> -        return 3;
>> +        ret = 3;
>> +        break;
>>      default:
>>          avpriv_request_sample(s, "Video codec (%x)", flv_codecid);
>>          par->codec_tag = flv_codecid;
>>      }
>>  
>> -    return 0;
>> +    if (par->codec_id != old_codec_id)
>> +        vstream->internal->need_context_update = 1;
> 
> If this occurs only for fuzzed samples then rejecting the change
> with a request for a sample seems better
>
> changing teh codec_id midstream like this could, seems problematic
> changing at at header time might be ok if that works better than not
> changing it for some real sample

This happens for almost every file, because at the beginning the codec_id
is AV_CODEC_ID_NONE. However, usually need_context_update is already
set from avformat_new_stream, so a change can be rejected, if this
is not the case. Patch doing that attached.

Best regards,
Andreas

Comments

Michael Niedermayer Nov. 24, 2016, 3:31 p.m. UTC | #1 
On Thu, Nov 24, 2016 at 01:37:17AM +0100, Andreas Cadhalpun wrote:
> On 23.11.2016 03:26, Michael Niedermayer wrote:
> > On Fri, Nov 04, 2016 at 10:28:20PM +0100, Andreas Cadhalpun wrote:
> >> Otherwise the codec context and codecpar might disagree on the codec id,
> >> triggering asserts in av_parser_parse2.
> >>
> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> >> ---
> >>  libavformat/flvdec.c | 16 ++++++++++++----
> >>  1 file changed, 12 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> >> index e53c345..4ba7fc8 100644
> >> --- a/libavformat/flvdec.c
> >> +++ b/libavformat/flvdec.c
> >> @@ -289,7 +289,9 @@ static int flv_same_video_codec(AVCodecParameters *vpar, int flags)
> >>  static int flv_set_video_codec(AVFormatContext *s, AVStream *vstream,
> >>                                 int flv_codecid, int read)
> >>  {
> >> +    int ret = 0;
> >>      AVCodecParameters *par = vstream->codecpar;
> >> +    enum AVCodecID old_codec_id = vstream->codecpar->codec_id;
> >>      switch (flv_codecid) {
> >>      case FLV_CODECID_H263:
> >>          par->codec_id = AV_CODEC_ID_FLV1;
> >> @@ -317,20 +319,26 @@ static int flv_set_video_codec(AVFormatContext *s, AVStream *vstream,
> >>              else
> >>                  avio_skip(s->pb, 1);
> >>          }
> >> -        return 1;     // 1 byte body size adjustment for flv_read_packet()
> >> +        ret = 1;     // 1 byte body size adjustment for flv_read_packet()
> >> +        break;
> >>      case FLV_CODECID_H264:
> >>          par->codec_id = AV_CODEC_ID_H264;
> >>          vstream->need_parsing = AVSTREAM_PARSE_HEADERS;
> >> -        return 3;     // not 4, reading packet type will consume one byte
> >> +        ret = 3;     // not 4, reading packet type will consume one byte
> >> +        break;
> >>      case FLV_CODECID_MPEG4:
> >>          par->codec_id = AV_CODEC_ID_MPEG4;
> >> -        return 3;
> >> +        ret = 3;
> >> +        break;
> >>      default:
> >>          avpriv_request_sample(s, "Video codec (%x)", flv_codecid);
> >>          par->codec_tag = flv_codecid;
> >>      }
> >>  
> >> -    return 0;
> >> +    if (par->codec_id != old_codec_id)
> >> +        vstream->internal->need_context_update = 1;
> > 
> > If this occurs only for fuzzed samples then rejecting the change
> > with a request for a sample seems better
> >
> > changing teh codec_id midstream like this could, seems problematic
> > changing at at header time might be ok if that works better than not
> > changing it for some real sample
> 
> This happens for almost every file, because at the beginning the codec_id
> is AV_CODEC_ID_NONE. However, usually need_context_update is already
> set from avformat_new_stream, so a change can be rejected, if this
> is not the case. Patch doing that attached.

the code i tested prviously checked for AV_CODEC_ID_NONE but this is
probably ok too

thx
8...]
Andreas Cadhalpun Nov. 24, 2016, 11:44 p.m. UTC | #2 
On 24.11.2016 16:31, Michael Niedermayer wrote:
> On Thu, Nov 24, 2016 at 01:37:17AM +0100, Andreas Cadhalpun wrote:
>> On 23.11.2016 03:26, Michael Niedermayer wrote:
>>> On Fri, Nov 04, 2016 at 10:28:20PM +0100, Andreas Cadhalpun wrote:
>>>> Otherwise the codec context and codecpar might disagree on the codec id,
>>>> triggering asserts in av_parser_parse2.
>>>>
>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>>>> ---
>>>>  libavformat/flvdec.c | 16 ++++++++++++----
>>>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
>>>> index e53c345..4ba7fc8 100644
>>>> --- a/libavformat/flvdec.c
>>>> +++ b/libavformat/flvdec.c
>>>> @@ -289,7 +289,9 @@ static int flv_same_video_codec(AVCodecParameters *vpar, int flags)
>>>>  static int flv_set_video_codec(AVFormatContext *s, AVStream *vstream,
>>>>                                 int flv_codecid, int read)
>>>>  {
>>>> +    int ret = 0;
>>>>      AVCodecParameters *par = vstream->codecpar;
>>>> +    enum AVCodecID old_codec_id = vstream->codecpar->codec_id;
>>>>      switch (flv_codecid) {
>>>>      case FLV_CODECID_H263:
>>>>          par->codec_id = AV_CODEC_ID_FLV1;
>>>> @@ -317,20 +319,26 @@ static int flv_set_video_codec(AVFormatContext *s, AVStream *vstream,
>>>>              else
>>>>                  avio_skip(s->pb, 1);
>>>>          }
>>>> -        return 1;     // 1 byte body size adjustment for flv_read_packet()
>>>> +        ret = 1;     // 1 byte body size adjustment for flv_read_packet()
>>>> +        break;
>>>>      case FLV_CODECID_H264:
>>>>          par->codec_id = AV_CODEC_ID_H264;
>>>>          vstream->need_parsing = AVSTREAM_PARSE_HEADERS;
>>>> -        return 3;     // not 4, reading packet type will consume one byte
>>>> +        ret = 3;     // not 4, reading packet type will consume one byte
>>>> +        break;
>>>>      case FLV_CODECID_MPEG4:
>>>>          par->codec_id = AV_CODEC_ID_MPEG4;
>>>> -        return 3;
>>>> +        ret = 3;
>>>> +        break;
>>>>      default:
>>>>          avpriv_request_sample(s, "Video codec (%x)", flv_codecid);
>>>>          par->codec_tag = flv_codecid;
>>>>      }
>>>>  
>>>> -    return 0;
>>>> +    if (par->codec_id != old_codec_id)
>>>> +        vstream->internal->need_context_update = 1;
>>>
>>> If this occurs only for fuzzed samples then rejecting the change
>>> with a request for a sample seems better
>>>
>>> changing teh codec_id midstream like this could, seems problematic
>>> changing at at header time might be ok if that works better than not
>>> changing it for some real sample
>>
>> This happens for almost every file, because at the beginning the codec_id
>> is AV_CODEC_ID_NONE. However, usually need_context_update is already
>> set from avformat_new_stream, so a change can be rejected, if this
>> is not the case. Patch doing that attached.
> 
> the code i tested prviously checked for AV_CODEC_ID_NONE

Only checking for AV_CODEC_ID_NONE is not sufficient, though, because it
does not guarantee that a newly set codec_id will actually be propagated
to the internal context. This only happens if need_context_update is set.

> but this is probably ok too

Pushed.

Do you think the ogg parsers should be handled similarly?
Because I'm similarly not sure if changing the codec parameters midstream
works for these.

Best regards,
Andreas
diff mbox

Patch

From 2561ebee3efd4e9b4b9ade04caa8ce7b79e0ac03 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
Date: Fri, 4 Nov 2016 21:37:13 +0100
Subject: [PATCH] flvdec: require need_context_update when changing codec id

Otherwise the codec context and codecpar might disagree on the codec id,
triggering asserts in av_parser_parse2.

Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavformat/flvdec.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
index 3812994..39e2142 100644
--- a/libavformat/flvdec.c
+++ b/libavformat/flvdec.c
@@ -291,7 +291,9 @@  static int flv_same_video_codec(AVCodecParameters *vpar, int flags)
 static int flv_set_video_codec(AVFormatContext *s, AVStream *vstream,
                                int flv_codecid, int read)
 {
+    int ret = 0;
     AVCodecParameters *par = vstream->codecpar;
+    enum AVCodecID old_codec_id = vstream->codecpar->codec_id;
     switch (flv_codecid) {
     case FLV_CODECID_H263:
         par->codec_id = AV_CODEC_ID_FLV1;
@@ -319,20 +321,28 @@  static int flv_set_video_codec(AVFormatContext *s, AVStream *vstream,
             else
                 avio_skip(s->pb, 1);
         }
-        return 1;     // 1 byte body size adjustment for flv_read_packet()
+        ret = 1;     // 1 byte body size adjustment for flv_read_packet()
+        break;
     case FLV_CODECID_H264:
         par->codec_id = AV_CODEC_ID_H264;
         vstream->need_parsing = AVSTREAM_PARSE_HEADERS;
-        return 3;     // not 4, reading packet type will consume one byte
+        ret = 3;     // not 4, reading packet type will consume one byte
+        break;
     case FLV_CODECID_MPEG4:
         par->codec_id = AV_CODEC_ID_MPEG4;
-        return 3;
+        ret = 3;
+        break;
     default:
         avpriv_request_sample(s, "Video codec (%x)", flv_codecid);
         par->codec_tag = flv_codecid;
     }
 
-    return 0;
+    if (!vstream->internal->need_context_update && par->codec_id != old_codec_id) {
+        avpriv_request_sample(s, "Changing the codec id midstream");
+        return AVERROR_PATCHWELCOME;
+    }
+
+    return ret;
 }
 
 static int amf_get_string(AVIOContext *ioc, char *buffer, int buffsize)
@@ -547,7 +557,9 @@  static int amf_parse_object(AVFormatContext *s, AVStream *astream,
                     st->codecpar->codec_id = AV_CODEC_ID_TEXT;
                 } else if (flv->trust_metadata) {
                     if (!strcmp(key, "videocodecid") && vpar) {
-                        flv_set_video_codec(s, vstream, num_val, 0);
+                        int ret = flv_set_video_codec(s, vstream, num_val, 0);
+                        if (ret < 0)
+                            return ret;
                     } else if (!strcmp(key, "audiocodecid") && apar) {
                         int id = ((int)num_val) << FLV_AUDIO_CODECID_OFFSET;
                         flv_set_audio_codec(s, astream, apar, id);
@@ -1098,7 +1110,10 @@  retry_duration:
             avcodec_parameters_free(&par);
         }
     } else if (stream_type == FLV_STREAM_TYPE_VIDEO) {
-        size -= flv_set_video_codec(s, st, flags & FLV_VIDEO_CODECID_MASK, 1);
+        int ret = flv_set_video_codec(s, st, flags & FLV_VIDEO_CODECID_MASK, 1);
+        if (ret < 0)
+            return ret;
+        size -= ret;
     } else if (stream_type == FLV_STREAM_TYPE_DATA) {
         st->codecpar->codec_id = AV_CODEC_ID_TEXT;
     }
-- 
2.10.2