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

[Zane van Iperen]

On Sun, 2 Aug 2020 13:28:10 +0200

> > +    for (int i = 0; i < FF_ARRAY_ELEMS(fhdr->name); i++)
> > +        avio_w8(pb, fhdr->name[i]);
> 
> Are you aware of avio_write()?

...yes. I basically copied the reading code, which used AV_RL8.
I've changed it.

> > +static int argo_asf_write_header(AVFormatContext *s)
> > +{
> > +    AVCodecParameters  *par = s->streams[0]->codecpar;
> > +    ArgoASFFileHeader  fhdr = { 0 };
> > +    ArgoASFChunkHeader chdr = { 0 };
> 
> The initializations are completely unnecessary (and actually the
> structures are, too).

I mean, technically yes, but it seems neater to fill out the structures
and then write them. I'd prefer to keep them personally.

If efficiency is paramount, I could AV_WLXX() to a buffer and
avio_write() that instead?

I can remove the initialisations though, that's no problem.

> > +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.

> > +
> > +    if (num_blocks > UINT32_MAX) {
> > +        av_log(s, AV_LOG_ERROR,
> > +               "Block count %"PRId64" invalid for ASF, output file will be broken\n",
> > +               num_blocks);
> 
> If the output will be broken, why don't you error out?
> 

Because... I forgot to. Is fixed.