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

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

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

> Paul B Mahol:
> > On Sun, Sep 10, 2023 at 2:54 PM Andreas Rheinhardt <
> > andreas.rheinhardt@outlook.com> wrote:
> >
> >> Paul B Mahol:
> >>> On Sun, Sep 10, 2023 at 2:18 PM Andreas Rheinhardt <
> >>> andreas.rheinhardt@outlook.com> wrote:
> >>>
> >>>> Paul B Mahol:
> >>>>> On Sun, Sep 10, 2023 at 2:06 PM Andreas Rheinhardt <
> >>>>> andreas.rheinhardt@outlook.com> wrote:
> >>>>>
> >>>>>> Paul B Mahol:
> >>>>>>> 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.
> >>>>>>>
> >>>>>>
> >>>>>> avio_seek() internally translates any avio_seek() with SEEK_CUR to
> an
> >>>>>> absolute seek (as required by the seek callbacks) and does not care
> >>>>>> about whether it is seeking forward or backward.
> >>>>>>
> >>>>>
> >>>>> Irrelevant. Seeking needs enough cache to work on non-seekable input.
> >>>>>
> >>>>
> >>>> Given that avio_skip() is just a wrapper around avio_seek(), the
> >>>> behaviour will not change even when the input is non-seekable.
> >>>>
> >>>
> >>> What you are trying to sell?
> >>>
> >>> That seeking works with unseekable input?
> >>>
> >>
> >> Seeking forward works for unseekable input by reading data and
> >> discarding it (that is how avio_seek() works). But actually we don't
> >> need that, all we need is that avio_skip() is just a wrapper around
> >> avio_seek(), so that the two forms are equivalent.
> >>
> >
> > Patches welcome, just do not break unseekable demuxing.
> >
>
> ? You want me to send a separate patch avoiding the unnecessary
> avio_seek() and switching to avio_seek() from avio_skip()? Why not apply
> it now?
>

I prefer avio_skip() solution.
And would prefer to stay that way,
It is irrelevant how underlying code call is implemented. Otherwise I'm not
code nazi.


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