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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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?

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