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

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