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

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