[FFmpeg-devel] [PATCH] avformat/scd: add demuxer

[Zane van Iperen]

On 23/11/21 16:18, Peter Ross wrote:

>>>> index cbfadcb639..1054ac9667 100644
>>>> --- a/libavformat/allformats.c
>>>> +++ b/libavformat/allformats.c
>>>> @@ -392,6 +392,7 @@ extern const AVOutputFormat ff_sbc_muxer;
>>>>    extern const AVInputFormat  ff_sbg_demuxer;
>>>>    extern const AVInputFormat  ff_scc_demuxer;
>>>>    extern const AVOutputFormat ff_scc_muxer;
>>>> +extern const AVInputFormat  ff_scd_demuxer;
>>>>    extern const AVInputFormat  ff_sdp_demuxer;
>>>>    extern const AVInputFormat  ff_sdr2_demuxer;
>>>>    extern const AVInputFormat  ff_sds_demuxer;
> 
> the indentation here is inconsistent.
> 

I think there may be something wrong with your viewer, all the indentation looks fine to me.

Does it look fine here? https://ffmpeg.org/pipermail/ffmpeg-devel/2021-November/287508.html


>>>> +static int scd_read_offsets(AVFormatContext *s)
>>>> +{
>>>> +    int64_t ret;
>>>> +    SCDDemuxContext  *ctx = s->priv_data;
>>>> +    uint8_t buf[SCD_OFFSET_HEADER_SIZE];
>>>> +
>>>> +    if ((ret = avio_read(s->pb, buf, SCD_OFFSET_HEADER_SIZE)) < 0)
>>>> +        return ret;
>>>> +
>>>> +    ctx->hdr.table0.count  = AV_RB16(buf +  0);
>>>> +    ctx->hdr.table1.count  = AV_RB16(buf +  2);
>>>> +    ctx->hdr.table2.count  = AV_RB16(buf +  4);
>>>> +    ctx->hdr.unk2          = AV_RB16(buf +  6);
>>>> +    ctx->hdr.table0.offset = AV_RB32(buf +  8);
>>>> +    ctx->hdr.table1.offset = AV_RB32(buf + 12);
>>>> +    ctx->hdr.table2.offset = AV_RB32(buf + 16);
>>>> +    ctx->hdr.unk3          = AV_RB32(buf + 20);
>>>> +    ctx->hdr.unk4          = AV_RB32(buf + 24);
> 
> is there any reason why you read the values into buf?
> 
> why not use avio_rb16/32(pb) to directly read the values. this is how other demuxers do it.
> also use avio_skip(pb, xxx) to skip over the unknown values.
> this saves having to define the structures and defining xxx_HEADER_SIZE.
> 

#notalldemuxers - See argo_{asf,cvg,brp}, pp_bnk, kvag, alp, etc.

Jokes aside, I've always avoided avio_rbxx() because of the potential need for error handling.
I find it nicer to do one big read and AV_RBXX() it out, than avio_rbxx() each field.

As for structures and xxx_HEADER_SIZE - I find it's helpful for future documentation and
debugging purposes. It's nice to be able to look at the header structure when debugging,
for example.

The `buf + XX` also makes it clear what the field offsets are.

>>>> +
>>>> +    track->length       = AV_RB32(buf +  0);
>>>> +    track->num_channels = AV_RB32(buf +  4);
>>>> +    track->sample_rate  = AV_RB32(buf +  8);
>>>> +    track->data_type    = AV_RB32(buf + 12);
>>>> +    track->loop_start   = AV_RB32(buf + 16);
>>>> +    track->loop_end     = AV_RB32(buf + 20);
>>>> +    track->data_offset  = AV_RB32(buf + 24);
>>>> +    track->aux_count    = AV_RB32(buf + 28);
> 
> ditto
> 

Ditto to your ditto.