[FFmpeg-devel] [PATCH 3/3] avcodec/packet: change public function and struct size parameter types to size_t

[Andreas Rheinhardt]

James Almer:
> On 5/31/2020 4:34 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 5/31/2020 4:11 PM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> On 5/31/2020 2:58 PM, Andreas Rheinhardt wrote:>> (Anyway, when this function is switched to size_t,
>>>>>> the correct error would be AVERROR(ERANGE). It is actually already the
>>>>>> correct error in case size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE.)
>>>>>> 3. That's unfortunately only a tiny fraction of the work to do before
>>>>>> this switch can happen:
>>>>>> a) libavcodec/mjpega_dump_header_bsf.c contains the following line:
>>>>>>      for (i = 0; i < in->size - 1; i++) {
>>>>>> If in->size == 0, then this will not have the desired effect, because
>>>>>> the subtraction now wraps around. There are probably more places like
>>>>>> this (and overflow checks that don't work as expected any more). We
>>>>>> would need to find them all and port them.
>>>>>
>>>>> Empty packets are considered flush packets, right? Guess we should be
>>>>> rejecting packets where pkt->size == 0 && pkt->data != NULL in
>>>>> av_bsf_send_packet(), same as we do in avcodec_send_packet(). I'll send
>>>>> a patch for that later.
>>>>>
>>>> The only way one can signal flushing to a bsf is by using av_bsf_flush.
>>>> Empty packets (data == NULL and no side_data) are considered eof.
>>>
>>> Yes, that's what i meant. The entire codebase uses flush, drain and EOF
>>> interchangeably in some cases.
>>>
>>>> Packets with data != NULL, size = 0 and no side data are currently
>>>> considered normal packets (a possible use-case would be to send some
>>>> timestamps to the bsf, although no bsf currently makes use of this; but
>>>> who knows what needs a future bsf has).
>>>
>>> data != NULL && size == 0 should IMO not be accepted. If we want to
>>> signal that we're feeding the bsf something like timestamps only, we'd
>>> have to find a different way to do it than setting pkt->data to some
>>> bogus value that's not NULL in order to bypass the IS_EMPTY() check.
>>>
>> Bogus value? You seem to believe that pkt->data just points to somewhere
>> in memory; in a valid packet it points to a buffer of size pkt->size +
>> AV_INPUT_BUFFER_PADDING_SIZE (even when pkt->size == 0).
>> av_packet_make_refcounted creates such packets if the input packet
>> doesn't have data
> 
> By bogus i mean a value that has no meaning for the packet you'd be
> feeding the bsf with seeing it's "empty". It doesn't matter if it points
> to a real buffer of only input padding bytes size, it's still a pointer
> that will not be accessed because the packet size is reported as 0.
> If we want to signal a packet with something like metadata only, then
> IMO it should be done in a different manner.
> 
>> (and the h264_mp4toannexb bsf currently relies on this).
> 
> That bsf expects a packet with size 0? Why? Empty packets with no side
> data were not meant to be propagated.

It relies on pkt->data always pointing to a buffer that has at least
four bytes of padding at the end.

- Andreas