| Message ID | 20180319154216.7728-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On 3/19/18, James Almer <jamrial@gmail.com> wrote: > Useful as well to quickly make a packet reference counted when it > isn't already so. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/avcodec.h | 11 +++++++++++ > libavcodec/avpacket.c | 24 ++++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > Will this ever be used?
On 3/19/2018 12:45 PM, Paul B Mahol wrote: > On 3/19/18, James Almer <jamrial@gmail.com> wrote: >> Useful as well to quickly make a packet reference counted when it >> isn't already so. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/avcodec.h | 11 +++++++++++ >> libavcodec/avpacket.c | 24 ++++++++++++++++++++++++ >> 2 files changed, 35 insertions(+) >> > > Will this ever be used? Probably? It's public API, downstream projects and users decide what they want to use based on their needs. And in our tree it can for example be used to simplify the change i made in a22c6a4796 and e91f0c4f8b, or in bitstream filters to allow changes to be made directly to the input packet's data.
On Mon, 19 Mar 2018 12:42:16 -0300 James Almer <jamrial@gmail.com> wrote: > Useful as well to quickly make a packet reference counted when it > isn't already so. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/avcodec.h | 11 +++++++++++ > libavcodec/avpacket.c | 24 ++++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > index a8322fb62a..a78017f1fb 100644 > --- a/libavcodec/avcodec.h > +++ b/libavcodec/avcodec.h > @@ -4518,6 +4518,17 @@ void av_packet_move_ref(AVPacket *dst, AVPacket *src); > */ > int av_packet_copy_props(AVPacket *dst, const AVPacket *src); > > +/** > + * Create a writable reference for the data described by a given packet, > + * avoiding data copy if possible. > + * > + * @param pkt Packet whose data should be made writable. > + * > + * @return 0 on success, a negative AVERROR on failure. On failure, the > + * packet is unchanged. > + */ > +int av_packet_make_writable(AVPacket *pkt); > + > /** > * Convert valid timing fields (timestamps / durations) in a packet from one > * timebase to another. Timestamps with unknown values (AV_NOPTS_VALUE) will be > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > index fe8113ab76..0693ca6f62 100644 > --- a/libavcodec/avpacket.c > +++ b/libavcodec/avpacket.c > @@ -652,6 +652,30 @@ void av_packet_move_ref(AVPacket *dst, AVPacket *src) > src->size = 0; > } > > +int av_packet_make_writable(AVPacket *pkt) > +{ > + AVBufferRef *buf = NULL; > + int ret; > + > + if (pkt->buf && av_buffer_is_writable(pkt->buf)) > + return 0; > + > + if (!pkt->data) > + return AVERROR(EINVAL); > + > + ret = packet_alloc(&buf, pkt->size); > + if (ret < 0) > + return ret; > + if (pkt->size) > + memcpy(buf->data, pkt->data, pkt->size); > + > + av_buffer_unref(&pkt->buf); > + pkt->buf = buf; > + pkt->data = buf->data; > + > + return 0; > +} > + > void av_packet_rescale_ts(AVPacket *pkt, AVRational src_tb, AVRational dst_tb) > { > if (pkt->pts != AV_NOPTS_VALUE) Why not just call av_buffer_make_writable()? This code seems fine too, though.
On 3/19/2018 1:32 PM, wm4 wrote: > On Mon, 19 Mar 2018 12:42:16 -0300 > James Almer <jamrial@gmail.com> wrote: > >> Useful as well to quickly make a packet reference counted when it >> isn't already so. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/avcodec.h | 11 +++++++++++ >> libavcodec/avpacket.c | 24 ++++++++++++++++++++++++ >> 2 files changed, 35 insertions(+) >> >> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h >> index a8322fb62a..a78017f1fb 100644 >> --- a/libavcodec/avcodec.h >> +++ b/libavcodec/avcodec.h >> @@ -4518,6 +4518,17 @@ void av_packet_move_ref(AVPacket *dst, AVPacket *src); >> */ >> int av_packet_copy_props(AVPacket *dst, const AVPacket *src); >> >> +/** >> + * Create a writable reference for the data described by a given packet, >> + * avoiding data copy if possible. >> + * >> + * @param pkt Packet whose data should be made writable. >> + * >> + * @return 0 on success, a negative AVERROR on failure. On failure, the >> + * packet is unchanged. >> + */ >> +int av_packet_make_writable(AVPacket *pkt); >> + >> /** >> * Convert valid timing fields (timestamps / durations) in a packet from one >> * timebase to another. Timestamps with unknown values (AV_NOPTS_VALUE) will be >> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >> index fe8113ab76..0693ca6f62 100644 >> --- a/libavcodec/avpacket.c >> +++ b/libavcodec/avpacket.c >> @@ -652,6 +652,30 @@ void av_packet_move_ref(AVPacket *dst, AVPacket *src) >> src->size = 0; >> } >> >> +int av_packet_make_writable(AVPacket *pkt) >> +{ >> + AVBufferRef *buf = NULL; >> + int ret; >> + >> + if (pkt->buf && av_buffer_is_writable(pkt->buf)) >> + return 0; >> + >> + if (!pkt->data) >> + return AVERROR(EINVAL); >> + >> + ret = packet_alloc(&buf, pkt->size); >> + if (ret < 0) >> + return ret; >> + if (pkt->size) >> + memcpy(buf->data, pkt->data, pkt->size); >> + >> + av_buffer_unref(&pkt->buf); >> + pkt->buf = buf; >> + pkt->data = buf->data; >> + >> + return 0; >> +} >> + >> void av_packet_rescale_ts(AVPacket *pkt, AVRational src_tb, AVRational dst_tb) >> { >> if (pkt->pts != AV_NOPTS_VALUE) > > Why not just call av_buffer_make_writable()? This code seems fine too, > though. That would require duplicating all or most of these checks every time you need to make sure a packet is writable. pkt->buf may be NULL, pkt->buf may already be writable, pkt->data may be NULL, the like. I figured a helper function like this where you just call one function that is "make this packet writable, whatever the current state is" would come in handy and simplify some parts of the tree. See the two commits i mentioned in my reply to Paul, or the mpeg4_unpack_bframes patch i sent last night.
On 3/19/2018 1:49 PM, James Almer wrote: > On 3/19/2018 1:32 PM, wm4 wrote: >> On Mon, 19 Mar 2018 12:42:16 -0300 >> James Almer <jamrial@gmail.com> wrote: >> >>> Useful as well to quickly make a packet reference counted when it >>> isn't already so. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavcodec/avcodec.h | 11 +++++++++++ >>> libavcodec/avpacket.c | 24 ++++++++++++++++++++++++ >>> 2 files changed, 35 insertions(+) >>> >>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h >>> index a8322fb62a..a78017f1fb 100644 >>> --- a/libavcodec/avcodec.h >>> +++ b/libavcodec/avcodec.h >>> @@ -4518,6 +4518,17 @@ void av_packet_move_ref(AVPacket *dst, AVPacket *src); >>> */ >>> int av_packet_copy_props(AVPacket *dst, const AVPacket *src); >>> >>> +/** >>> + * Create a writable reference for the data described by a given packet, >>> + * avoiding data copy if possible. >>> + * >>> + * @param pkt Packet whose data should be made writable. >>> + * >>> + * @return 0 on success, a negative AVERROR on failure. On failure, the >>> + * packet is unchanged. >>> + */ >>> +int av_packet_make_writable(AVPacket *pkt); >>> + >>> /** >>> * Convert valid timing fields (timestamps / durations) in a packet from one >>> * timebase to another. Timestamps with unknown values (AV_NOPTS_VALUE) will be >>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>> index fe8113ab76..0693ca6f62 100644 >>> --- a/libavcodec/avpacket.c >>> +++ b/libavcodec/avpacket.c >>> @@ -652,6 +652,30 @@ void av_packet_move_ref(AVPacket *dst, AVPacket *src) >>> src->size = 0; >>> } >>> >>> +int av_packet_make_writable(AVPacket *pkt) >>> +{ >>> + AVBufferRef *buf = NULL; >>> + int ret; >>> + >>> + if (pkt->buf && av_buffer_is_writable(pkt->buf)) >>> + return 0; >>> + >>> + if (!pkt->data) >>> + return AVERROR(EINVAL); >>> + >>> + ret = packet_alloc(&buf, pkt->size); >>> + if (ret < 0) >>> + return ret; >>> + if (pkt->size) >>> + memcpy(buf->data, pkt->data, pkt->size); >>> + >>> + av_buffer_unref(&pkt->buf); >>> + pkt->buf = buf; >>> + pkt->data = buf->data; >>> + >>> + return 0; >>> +} >>> + >>> void av_packet_rescale_ts(AVPacket *pkt, AVRational src_tb, AVRational dst_tb) >>> { >>> if (pkt->pts != AV_NOPTS_VALUE) >> >> Why not just call av_buffer_make_writable()? This code seems fine too, >> though. > > That would require duplicating all or most of these checks every time > you need to make sure a packet is writable. pkt->buf may be NULL, > pkt->buf may already be writable, pkt->data may be NULL, the like. > > I figured a helper function like this where you just call one function > that is "make this packet writable, whatever the current state is" would > come in handy and simplify some parts of the tree. See the two commits i > mentioned in my reply to Paul, or the mpeg4_unpack_bframes patch i sent > last night. I'll apply this later today or tomorrow unless someone objects.
On 3/21/2018 2:14 PM, James Almer wrote: > On 3/19/2018 1:49 PM, James Almer wrote: >> On 3/19/2018 1:32 PM, wm4 wrote: >>> On Mon, 19 Mar 2018 12:42:16 -0300 >>> James Almer <jamrial@gmail.com> wrote: >>> >>>> Useful as well to quickly make a packet reference counted when it >>>> isn't already so. >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavcodec/avcodec.h | 11 +++++++++++ >>>> libavcodec/avpacket.c | 24 ++++++++++++++++++++++++ >>>> 2 files changed, 35 insertions(+) >>>> >>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h >>>> index a8322fb62a..a78017f1fb 100644 >>>> --- a/libavcodec/avcodec.h >>>> +++ b/libavcodec/avcodec.h >>>> @@ -4518,6 +4518,17 @@ void av_packet_move_ref(AVPacket *dst, AVPacket *src); >>>> */ >>>> int av_packet_copy_props(AVPacket *dst, const AVPacket *src); >>>> >>>> +/** >>>> + * Create a writable reference for the data described by a given packet, >>>> + * avoiding data copy if possible. >>>> + * >>>> + * @param pkt Packet whose data should be made writable. >>>> + * >>>> + * @return 0 on success, a negative AVERROR on failure. On failure, the >>>> + * packet is unchanged. >>>> + */ >>>> +int av_packet_make_writable(AVPacket *pkt); >>>> + >>>> /** >>>> * Convert valid timing fields (timestamps / durations) in a packet from one >>>> * timebase to another. Timestamps with unknown values (AV_NOPTS_VALUE) will be >>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>> index fe8113ab76..0693ca6f62 100644 >>>> --- a/libavcodec/avpacket.c >>>> +++ b/libavcodec/avpacket.c >>>> @@ -652,6 +652,30 @@ void av_packet_move_ref(AVPacket *dst, AVPacket *src) >>>> src->size = 0; >>>> } >>>> >>>> +int av_packet_make_writable(AVPacket *pkt) >>>> +{ >>>> + AVBufferRef *buf = NULL; >>>> + int ret; >>>> + >>>> + if (pkt->buf && av_buffer_is_writable(pkt->buf)) >>>> + return 0; >>>> + >>>> + if (!pkt->data) >>>> + return AVERROR(EINVAL); >>>> + >>>> + ret = packet_alloc(&buf, pkt->size); >>>> + if (ret < 0) >>>> + return ret; >>>> + if (pkt->size) >>>> + memcpy(buf->data, pkt->data, pkt->size); >>>> + >>>> + av_buffer_unref(&pkt->buf); >>>> + pkt->buf = buf; >>>> + pkt->data = buf->data; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> void av_packet_rescale_ts(AVPacket *pkt, AVRational src_tb, AVRational dst_tb) >>>> { >>>> if (pkt->pts != AV_NOPTS_VALUE) >>> >>> Why not just call av_buffer_make_writable()? This code seems fine too, >>> though. >> >> That would require duplicating all or most of these checks every time >> you need to make sure a packet is writable. pkt->buf may be NULL, >> pkt->buf may already be writable, pkt->data may be NULL, the like. >> >> I figured a helper function like this where you just call one function >> that is "make this packet writable, whatever the current state is" would >> come in handy and simplify some parts of the tree. See the two commits i >> mentioned in my reply to Paul, or the mpeg4_unpack_bframes patch i sent >> last night. > > I'll apply this later today or tomorrow unless someone objects. Pushed.
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index a8322fb62a..a78017f1fb 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -4518,6 +4518,17 @@ void av_packet_move_ref(AVPacket *dst, AVPacket *src); */ int av_packet_copy_props(AVPacket *dst, const AVPacket *src); +/** + * Create a writable reference for the data described by a given packet, + * avoiding data copy if possible. + * + * @param pkt Packet whose data should be made writable. + * + * @return 0 on success, a negative AVERROR on failure. On failure, the + * packet is unchanged. + */ +int av_packet_make_writable(AVPacket *pkt); + /** * Convert valid timing fields (timestamps / durations) in a packet from one * timebase to another. Timestamps with unknown values (AV_NOPTS_VALUE) will be diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index fe8113ab76..0693ca6f62 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -652,6 +652,30 @@ void av_packet_move_ref(AVPacket *dst, AVPacket *src) src->size = 0; } +int av_packet_make_writable(AVPacket *pkt) +{ + AVBufferRef *buf = NULL; + int ret; + + if (pkt->buf && av_buffer_is_writable(pkt->buf)) + return 0; + + if (!pkt->data) + return AVERROR(EINVAL); + + ret = packet_alloc(&buf, pkt->size); + if (ret < 0) + return ret; + if (pkt->size) + memcpy(buf->data, pkt->data, pkt->size); + + av_buffer_unref(&pkt->buf); + pkt->buf = buf; + pkt->data = buf->data; + + return 0; +} + void av_packet_rescale_ts(AVPacket *pkt, AVRational src_tb, AVRational dst_tb) { if (pkt->pts != AV_NOPTS_VALUE)
Useful as well to quickly make a packet reference counted when it isn't already so. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/avcodec.h | 11 +++++++++++ libavcodec/avpacket.c | 24 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+)