[FFmpeg-devel] [PATCH] avformat/utils: use the existing packet reference when parsing complete frames

[James Almer]

On 4/12/2018 6:52 PM, wm4 wrote:
> On Thu, 12 Apr 2018 16:22:01 -0300
> James Almer <jamrial at gmail.com> wrote:
> 
>> On 4/12/2018 3:59 PM, wm4 wrote:
>>> On Thu, 12 Apr 2018 15:34:29 -0300
>>> James Almer <jamrial at gmail.com> wrote:
>>>   
>>>> If the parser returns full frames, then the output pointer retured by
>>>> av_parser_parse2() is guaranteed to point to data contained in the
>>>> input packet's buffer.
>>>>
>>>> Create a new reference to said buffer in that case, to avoid
>>>> unnecessary data copy when queueing the packet later in the function.
>>>>
>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>> ---
>>>>  libavformat/utils.c | 23 ++++++++++++++++++++---
>>>>  1 file changed, 20 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>>>> index 3e482a3bbc..8ad2ef4d70 100644
>>>> --- a/libavformat/utils.c
>>>> +++ b/libavformat/utils.c
>>>> @@ -1471,6 +1471,22 @@ static int parse_packet(AVFormatContext *s, AVPacket *pkt, int stream_index)
>>>>          if (!out_pkt.size)
>>>>              continue;
>>>>  
>>>> +        if (pkt->buf && out_pkt.data == pkt->data) {
>>>> +            /* reference pkt->buf only when out_pkt.data is guaranteed to point
>>>> +             * to data in it and not in the parser's internal buffer. */
>>>> +            /* XXX: Ensure this is the case with all parsers when the muxer sets
>>>> +             * PARSER_FLAG_COMPLETE_FRAMES and check for that instead? */
>>>> +            out_pkt.buf = av_buffer_ref(pkt->buf);
>>>> +            if (!out_pkt.buf) {
>>>> +                ret = AVERROR(ENOMEM);
>>>> +                goto fail;
>>>> +            }
>>>> +        } else {
>>>> +            ret = av_packet_make_refcounted(&out_pkt);
>>>> +            if (ret < 0)
>>>> +                goto fail;
>>>> +        }
>>>> +
>>>>          if (pkt->side_data) {
>>>>              out_pkt.side_data       = pkt->side_data;
>>>>              out_pkt.side_data_elems = pkt->side_data_elems;
>>>> @@ -1511,10 +1527,11 @@ static int parse_packet(AVFormatContext *s, AVPacket *pkt, int stream_index)
>>>>  
>>>>          ret = ff_packet_list_put(&s->internal->parse_queue,
>>>>                                   &s->internal->parse_queue_end,
>>>> -                                 &out_pkt, FF_PACKETLIST_FLAG_REF_PACKET);
>>>> -        av_packet_unref(&out_pkt);
>>>> -        if (ret < 0)
>>>> +                                 &out_pkt, 0);
>>>> +        if (ret < 0) {
>>>> +            av_packet_unref(&out_pkt);
>>>>              goto fail;
>>>> +        }
>>>>      }
>>>>  
>>>>      /* end of the stream => close and free the parser */  
>>>
>>> For which cases is this?  
>>
>> Pretty much every single packet from a demuxer that sets
>> AVSTREAM_PARSE_HEADERS, meaning every non-raw demuxer with a couple
>> exceptions.
>>
>>> Maybe we should just make a new parser API (or
>>> reuse the BSF one), and make it cover the cases we care about.  
>>
>> I remember a new parser API that works on packets rather than raw data,
>> much like the new bsf API, was the plan at some point.
>> There's no lack of ideas in any case, just manpower in general.
> 
> To be honest I wish we just did away with this probing stuff and
> restrict libavformat to actually demuxing stuff.
> 
> Could we use that PARSE_HEADERS probably never changes packet contents?

AVSTREAM_PARSE_HEADERS is a constant AVStream->needs_parsing gets set to
by demuxers, which lavf/utils.c then uses to set
AVStream->AVParser->flags to PARSER_FLAG_COMPLETE_FRAMES, the actual
AVParser API flag (Which should probably get an AV prefix, btw).
Even then, not all parsers actually care about that flag being set, as
is the case with for example png, bmp, mlp until not too long ago, etc.
So at least right now no, we can't assume all packets will be parsed for
codec specific params then passed along untouched even if that flag is
set. I mentioned as much in the commented out chunk in this patch and
the reason I'm comparing the in and out data pointers.