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

[Andreas Rheinhardt]

Paul B Mahol:
> On Sun, Sep 10, 2023 at 1:59 PM Andreas Rheinhardt <
> andreas.rheinhardt at outlook.com> wrote:
> 
>> Paul B Mahol:
>>> On Wed, Sep 6, 2023 at 1:30 PM Andreas Rheinhardt <
>>> andreas.rheinhardt at outlook.com> wrote:
>>>
>>>> Paul B Mahol:
>>>>> On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
>>>>> andreas.rheinhardt at 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.

- Andreas