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

[wm4]

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?