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

Submitted by James Almer on April 12, 2018, 6:34 p.m.

Details

Message ID 20180412183429.7368-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer April 12, 2018, 6:34 p.m.
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@gmail.com>
---
 libavformat/utils.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

Comments

wm4 April 12, 2018, 6:59 p.m.
On Thu, 12 Apr 2018 15:34:29 -0300
James Almer <jamrial@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@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? Maybe we should just make a new parser API (or
reuse the BSF one), and make it cover the cases we care about.
James Almer April 12, 2018, 7:22 p.m.
On 4/12/2018 3:59 PM, wm4 wrote:
> On Thu, 12 Apr 2018 15:34:29 -0300
> James Almer <jamrial@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@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.
wm4 April 12, 2018, 9:52 p.m.
On Thu, 12 Apr 2018 16:22:01 -0300
James Almer <jamrial@gmail.com> wrote:

> On 4/12/2018 3:59 PM, wm4 wrote:
> > On Thu, 12 Apr 2018 15:34:29 -0300
> > James Almer <jamrial@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@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?
James Almer April 12, 2018, 10:09 p.m.
On 4/12/2018 6:52 PM, wm4 wrote:
> On Thu, 12 Apr 2018 16:22:01 -0300
> James Almer <jamrial@gmail.com> wrote:
> 
>> On 4/12/2018 3:59 PM, wm4 wrote:
>>> On Thu, 12 Apr 2018 15:34:29 -0300
>>> James Almer <jamrial@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@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.
Hendrik Leppkes April 12, 2018, 10:25 p.m.
On Thu, Apr 12, 2018 at 11:52 PM, wm4 <nfxjfg@googlemail.com> wrote:
>
> Could we use that PARSE_HEADERS probably never changes packet contents?

Not without reviewing all parsers, its not enforced, just a usage hint really.

- Hendrik
Michael Niedermayer April 13, 2018, 6:59 p.m.
On Thu, Apr 12, 2018 at 03:34:29PM -0300, James Almer 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@gmail.com>
> ---
>  libavformat/utils.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)

LGTM

thx

[...]
James Almer April 14, 2018, 12:20 a.m.
On 4/13/2018 3:59 PM, Michael Niedermayer wrote:
> On Thu, Apr 12, 2018 at 03:34:29PM -0300, James Almer 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@gmail.com>
>> ---
>>  libavformat/utils.c | 23 ++++++++++++++++++++---
>>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> LGTM
> 
> thx

Pushed.

Patch hide | download patch | download mbox

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 */