[FFmpeg-devel] [PATCH 4/6] avformat: add argo_asf muxer

[Andreas Rheinhardt]

Zane van Iperen:
> On Sun, 2 Aug 2020 14:14:45 +0200
> "Andreas Rheinhardt" <andreas.rheinhardt at gmail.com> wrote:
> 
>>
>> Zane van Iperen:
>>>>> +static int argo_asf_write_trailer(AVFormatContext *s)
>>>>> +{
>>>>> +    int64_t data_size, num_blocks;
>>>>> +
>>>>> +    data_size = avio_tell(s->pb) - (ASF_FILE_HEADER_SIZE + ASF_CHUNK_HEADER_SIZE);
>>>>> +    num_blocks = data_size / (17 * s->streams[0]->codecpar->channels);
>>>>> +
>>>>> +    av_assert0(data_size % (17 * s->streams[0]->codecpar->channels) == 0);
>>>>
>>>> You must not assert this as you have not have any check in place that
>>>> guarantees this.
>>>>
>>>> The comment in the demuxer and the value you assign to num_samples when
>>>> writing the header indicates to me that num_samples is supposed to be
>>>> always 32; yet there is no check for that in the demuxer and if it is a
>>>> value different from 32 and 33, then simply remuxing such a file can
>>>> trigger this assert.
>>>>
>>>
>>> You're right, num_samples should always be 32. I will add a check in
>>> the demuxer.
>>>
>>> The 17 comes from "control byte + (32 / 2)" (the frame size for a
>>> single channel), so adding that check should guarantee the assertion.
>>>
>>
>> Why? The packets sent to this muxer need not originate from the demuxer
>> via remuxing or from the encoder that you are about to add.
> 
> So, something like this basically:
> 
>     static int argo_asf_write_packet(AVFormatContext *s, AVPacket *pkt)
>     {
>         if (pkt->size != 17 && pkt->size != 34)
>             return AVERROR(EINVAL);
> 
>         avio_write(s->pb, pkt->data, pkt->size);
>         return 0;
>     }
> 
I think it is rather AVERROR_INVALIDDATA. But even then there are still
two problems: You allow 17 byte packets in case of stereo, yet in this
case you still assert that the data size is divisible by 34. And more
serious: Currently, the AVIOContext's position is incremented even if a
write error happened or if writing is not even attempted because of an
earlier write error, yet there is no guarantee that it always stays that
way.

For the record, AVStream.nb_frames contains the number of frames
successfully written. You could simply use that and ignore any
divisibility. Or you could check the sizes in a custom write_packet
function and still just use AVStream.nb_frames when writing the trailer.

- Andreas