[FFmpeg-devel] [PATCH v4 7/9] avformat/utils: Avoid copying packets unnecessarily

[Andreas Rheinhardt]

James Almer:
> On 9/27/2019 11:52 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 9/20/2019 5:39 PM, Andreas Rheinhardt wrote:
>>>> Up until now, read_frame_internal in avformat/utils.c uses a spare
>>>> packet on the stack that serves no real purpose: At no point in this
>>>> function is there a need for another packet besides the packet destined
>>>> for output:
>>>> 1. If the packet doesn't need a parser, but is output as is, the content
>>>> of the spare packet (that at this point contains a freshly read packet)
>>>> is simply copied into the output packet (via simple assignment, not
>>>> av_packet_move_ref, thereby confusing ownership).
>>>> 2. If the packet needs parsing, the spare packet will be reset after
>>>> parsing and any packets resulting from the packet read will be put into
>>>> a packet list; the output packet is not used here at all.
>>>> 3. If the stream should be discarded, the spare packet will be
>>>> unreferenced; the output packet is not used here at all either.
>>>>
>>>> Therefore the spare packet and the copies can be removed in principle.
>>>> In practice, one more thing needs to be taken care of: If ff_read_packet
>>>> failed, the output packet was not affected, now it is. But given that
>>>> ff_read_packet returns a blank (as if reset via av_packet_unref) packet
>>>> on failure, there is no problem from this side either.
>>>
>>> There's still the "if (!pktl && st->request_probe <= 0)" check in
>>> ff_read_packet(), which returns without unreferencing the packet.
>>>
>> And that's how it should be, because this is not failure. It is the
>> ordinary way to return from ff_read_packet() when reading was
>> successfull and the probing is unnecessary. In this case, there is no
>> need for the overhead of the packet list.
>>
>> - Andreas
> 
> It should return 0 rather than ret, then, if anything to be less
> confusing, but also because otherwise it depends on what last set that
> variable, and right now that's AVFormatInput->read_packet().
> 
> In any case, this one and the rest of the patchset pushed. Thanks.

I also found this strange, but I thought that this was intentional
(why else would one use two variables (ret and err) for the return
values?). But on closer inspection, it seems that all callers are fine
with returning 0 in this case:
- estimate_timings_from_pts() checks for whether the return value is
!= 0. But it seems that both the mpegts as well as the mpegps demuxer
(estimate_timings_from_pts() is only used with them) only return 0 on
success.
- asfrtp_parse_packet() also checks whether the return value is != 0,
but the asf demuxers never return a value > 0 on success, so this
seems fine, too.
- The calls in avidec.c are either unchecked or check for < 0.
- read_frame_internal() overwrites the return value with 0 on success.

So I made a patch to change this [1].

- Andreas

[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-September/250740.html

PS: All read_packet functions should return 0 on success according to
the doxy, but not all of them abide by this. Some demuxers (e.g. ogg)
return the size of the returned packet's data.