[FFmpeg-devel] [PATCH 3/4] lavf/movenc: Fail when codec tag is invalid for format

[James Almer]

On 8/27/2018 6:07 PM, John Stebbins wrote:
> On 08/27/2018 02:03 PM, James Almer wrote:
>> On 8/27/2018 5:48 PM, John Stebbins wrote:
>>> On 08/27/2018 01:29 PM, James Almer wrote:
>>>> On 8/27/2018 4:57 PM, John Stebbins wrote:
>>>>> Fixes ticket #6897
>>>>> ---
>>>>>  libavformat/movenc.c | 40 +++++++++++++++++++++++++++++-----------
>>>>>  1 file changed, 29 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>>>> index 8a3b651514..dd6281d210 100644
>>>>> --- a/libavformat/movenc.c
>>>>> +++ b/libavformat/movenc.c
>>>>> @@ -1589,6 +1589,26 @@ static const AVCodecTag codec_cover_image_tags[] = {
>>>>>      { AV_CODEC_ID_NONE, 0 },
>>>>>  };
>>>>>  
>>>>> +static int validate_codec_tag(const AVCodecTag *const *tags,
>>>>> +                              unsigned int tag, int codec_id)
>>>>> +{
>>>>> +    int i;
>>>>> +
>>>>> +    /**
>>>>> +     * Check that tag + id is in the table
>>>>> +     */
>>>>> +    for (i = 0; tags && tags[i]; i++) {
>>>>> +        const AVCodecTag *codec_tags = tags[i];
>>>>> +        while (codec_tags->id != AV_CODEC_ID_NONE) {
>>>>> +            if (codec_tags->tag == tag && codec_tags->id == codec_id) {
>>>> Make both tag checks case insensitive using avpriv_toupper4(), then
>>>> return codec_tags->tag instead of 1 if the check succeeds.
>>> I've never seen mismatched case in these tags, but sure, why not... there's plenty I haven't seen.
>> AV1 in IVF is AV01, wheres in mp4 it's av01. That's the case i had in
>> mind when requesting this.
> 
> Hmm, I should probably return codec_tags->tag in this case rather than tag since it is the expected capitalization for
> the container.  I'll have to fix that (again).

What's the status of this? With that change i think it should be good to
go in.

> 
>>
>>>>> +                return 1;
>>>>> +            }
>>>>> +            codec_tags++;
>>>>> +        }
>>>>> +    }
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>  static int mov_find_codec_tag(AVFormatContext *s, MOVTrack *track)
>>>>>  {
>>>>>      int tag;
>>>> Take the opportunity to change this, the ones in mov_get_codec_tag() and
>>>> in validate_codec_tag() to unsigned int, including the return types.
>>>> Codec tags in AVCodecTag are unsigned after all.
>>>>
>>>>> @@ -1596,23 +1616,21 @@ static int mov_find_codec_tag(AVFormatContext *s, MOVTrack *track)
>>>>>      if (is_cover_image(track->st))
>>>>>          return ff_codec_get_tag(codec_cover_image_tags, track->par->codec_id);
>>>>>  
>>>>> -    if (track->mode == MODE_MP4 || track->mode == MODE_PSP)
>>>>> -        tag = track->par->codec_tag;
>>>>> -    else if (track->mode == MODE_ISM)
>>>>> -        tag = track->par->codec_tag;
>>>>> -    else if (track->mode == MODE_IPOD) {
>>>>> +    if (track->mode == MODE_IPOD)
>>>>>          if (!av_match_ext(s->url, "m4a") &&
>>>>>              !av_match_ext(s->url, "m4v") &&
>>>>>              !av_match_ext(s->url, "m4b"))
>>>>>              av_log(s, AV_LOG_WARNING, "Warning, extension is not .m4a nor .m4v "
>>>>>                     "Quicktime/Ipod might not play the file\n");
>>>>> -        tag = track->par->codec_tag;
>>>>> -    } else if (track->mode & MODE_3GP)
>>>>> -        tag = track->par->codec_tag;
>>>>> -    else if (track->mode == MODE_F4V)
>>>>> -        tag = track->par->codec_tag;
>>>>> -    else
>>>>> +
>>>>> +    if (track->mode == MODE_MOV)
>>>>>          tag = mov_get_codec_tag(s, track);
>>>>> +    else
>>>>> +        if (!validate_codec_tag(s->oformat->codec_tag, track->par->codec_tag,
>>>>> +                                track->par->codec_id))
>>>>> +            tag = 0;
>>>>> +        else
>>>>> +            tag = track->par->codec_tag;
>>>> And of course make this simply
>>>>
>>>> tag = validate_codec_tag(...);
>>>>
>>>> Thanks.
>>>>
>>>>>  
>>>>>      return tag;
>>>>>  }
>>>>>
>>>
>>> All good suggestions, thanks.
>>>
>>>
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>