| Message ID | 20180412183429.7368-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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?
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.
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
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 [...]
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.
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 */
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(-)