Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer

[Paul B Mahol <onemda@gmail.com>]

On Sun, Sep 10, 2023 at 1:59 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> Paul B Mahol:
> > On Wed, Sep 6, 2023 at 1:30 PM Andreas Rheinhardt <
> > andreas.rheinhardt@outlook.com> wrote:
> >
> >> Paul B Mahol:
> >>> On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
> >>> andreas.rheinhardt@outlook.com> wrote:
> >>>
> >>>> Paul B Mahol:
> >>>>>
> >>>>> +    chunk_type = avio_rb32(pb);
> >>>>> +    chunk_size = avio_rb32(pb);
> >>>>
> >>>> You are not checking whether the chunk here exceeds its containing
> >> chunk.
> >>>>
> >>>>>
> >>>>> +    av_fast_malloc(&usm->header, &usm->header_size,
> >>>>> +                   chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
> >>>>> +    if (!usm->header)
> >>>>> +        return AVERROR(ENOMEM);
> >>>>
> >>>> The bytestream2 API does not rely on the buffer being padded at all.
> >>>>
> >>>>>
> >>>>> +    bytestream2_skip(&sgb, string_offset);
> >>>>
> >>>> This is unnecessary, because you seek with an absolute offset lateron
> >>>> anyway before using sgb.
> >>>>
> >>>>>
> >>>>> +        bytestream2_seek(&sgb, string_offset + offset, SEEK_SET);
> >>>>> +        while (bytestream2_get_bytes_left(&sgb) > 0) {
> >>>>> +            key[n] = bytestream2_get_byte(&sgb);
> >>>>> +            if (!key[n])
> >>>>> +                break;
> >>>>> +            if (n >= sizeof(key) - 1)
> >>>>> +                break;
> >>>>> +            n++;
> >>>>> +        }
> >>>>> +        key[n] = '\0';
> >>>>
> >>>> IMO this would be easier with strnlen(), avoiding sgb altogether.
> >>>> You would of course need to explicitly check that you are not
> >>>> overreading, but that is good practice anyway.
> >>>>
> >>>>>
> >>>>> +    chunk_start = avio_tell(pb);
> >>>>> +    avio_skip(pb, 1);
> >>>>> +    payload_offset = avio_r8(pb);
> >>>>> +    padding_size = avio_rb16(pb);
> >>>>> +    stream_index = avio_r8(pb);
> >>>>> +    avio_skip(pb, 2);
> >>>>> +    payload_type = avio_r8(pb);
> >>>>> +    frame_time = avio_rb32(pb);
> >>>>> +    frame_rate = avio_rb32(pb);
> >>>>> +    avio_skip(pb, 8);
> >>>>
> >>>> payload_offset and frame_time are set-but-unused; this might lead to
> >>>> compiler warnings.
> >>>>
> >>>>> +        if (usm->ch[is_audio][stream_index].used == 1) {
> >>>>> +            uint32_t pkt_size = chunk_size - (avio_tell(pb) -
> >>>> chunk_start);
> >>>>> +
> >>>>
> >>>> This is unnecessary: Unless we already had a read error, pkt_size is
> >>>> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
> >>>>
> >>>> (Notice that in case padding_size is > 0, it will be part of the
> packet
> >>>> with the current code; not sure if that is an issue.)
> >>>>
> >>>>>
> >>>>> +
> >>>>> +    avio_skip(pb, padding_size);
> >>>>> +    avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
> >>>>> +
> >>>>
> >>>> Simpler to just use avio_seek(pb, chunk_start + chunk_size, SEEK_SET);
> >>>>
> >>>
> >>> But input might not be seekable.
> >>>
> >>
> >> And? You know that avio_skip(pb, offset) is just avio_seek(pb, offset,
> >> SEEK_CUR)?
> >>
> >
> > And? Do you know that SEEK_SET is different from SEEK_CUR with positive
> > argument.
> >
>
> You are using SEEK_CUR with -avio_tell(pb), which effectively makes it
> an absolute seek.
>

Nope, I skip data only, not seeking backward ever.


>
> - Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".