| Message ID | 20170221231432.15759-1-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | 436f00b10c062b75c7aab276c4a7d64524bd0444 |
| Headers | show |
On Wed, 22 Feb 2017 00:14:32 +0100 Marton Balint <cus@passwd.hu> wrote: > This ensures that the wrapped avframe will not get reallocated later, which > would invalidate internal references such as extended data. > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavcodec/wrapped_avframe.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/wrapped_avframe.c b/libavcodec/wrapped_avframe.c > index 13c8d8a..1436032 100644 > --- a/libavcodec/wrapped_avframe.c > +++ b/libavcodec/wrapped_avframe.c > @@ -43,19 +43,31 @@ static int wrapped_avframe_encode(AVCodecContext *avctx, AVPacket *pkt, > const AVFrame *frame, int *got_packet) > { > AVFrame *wrapped = av_frame_clone(frame); > + uint8_t *data; > + int size = sizeof(*wrapped) + AV_INPUT_BUFFER_PADDING_SIZE; > > if (!wrapped) > return AVERROR(ENOMEM); > > - pkt->buf = av_buffer_create((uint8_t *)wrapped, sizeof(*wrapped), > + data = av_mallocz(size); > + if (!data) { > + av_frame_free(&wrapped); > + return AVERROR(ENOMEM); > + } > + > + pkt->buf = av_buffer_create(data, size, > wrapped_avframe_release_buffer, NULL, > AV_BUFFER_FLAG_READONLY); > if (!pkt->buf) { > av_frame_free(&wrapped); > + av_freep(&data); > return AVERROR(ENOMEM); > } > > - pkt->data = (uint8_t *)wrapped; > + av_frame_move_ref((AVFrame*)data, wrapped); > + av_frame_free(&wrapped); I think this could be done as just av_frame_ref((AVFrame*)data, frame); without calling av_frame_clone(), but it doesn't hurt either. (Changing it might be an optional, minor improvement.) > + > + pkt->data = data; > pkt->size = sizeof(*wrapped); > > pkt->flags |= AV_PKT_FLAG_KEY; Patch looks good, and I like it better than checking the codec ID. And now something in general: wrapped_avframe is a special-case, because it stores a pointer in what is supposed to be just a raw byte array. So it's always possible that AVPacket use with it breaks in tricky ways that is fine with normal codecs. To make wrapped_avframe absolutely safe, we could either introduce a separate AVBuffer field just for wrapped_avframe, or we could make side-data refcounted (like with AVFrame), and store the AVFrame in side-data, which would probably be slightly more robust. Maybe something to consider on the next ABI bump.
On Wed, 22 Feb 2017, wm4 wrote: > On Wed, 22 Feb 2017 00:14:32 +0100 > Marton Balint <cus@passwd.hu> wrote: > >> This ensures that the wrapped avframe will not get reallocated later, which >> would invalidate internal references such as extended data. >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libavcodec/wrapped_avframe.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/wrapped_avframe.c b/libavcodec/wrapped_avframe.c >> index 13c8d8a..1436032 100644 >> --- a/libavcodec/wrapped_avframe.c >> +++ b/libavcodec/wrapped_avframe.c >> @@ -43,19 +43,31 @@ static int wrapped_avframe_encode(AVCodecContext *avctx, AVPacket *pkt, >> const AVFrame *frame, int *got_packet) >> { >> AVFrame *wrapped = av_frame_clone(frame); >> + uint8_t *data; >> + int size = sizeof(*wrapped) + AV_INPUT_BUFFER_PADDING_SIZE; >> >> if (!wrapped) >> return AVERROR(ENOMEM); >> >> - pkt->buf = av_buffer_create((uint8_t *)wrapped, sizeof(*wrapped), >> + data = av_mallocz(size); >> + if (!data) { >> + av_frame_free(&wrapped); >> + return AVERROR(ENOMEM); >> + } >> + >> + pkt->buf = av_buffer_create(data, size, >> wrapped_avframe_release_buffer, NULL, >> AV_BUFFER_FLAG_READONLY); >> if (!pkt->buf) { >> av_frame_free(&wrapped); >> + av_freep(&data); >> return AVERROR(ENOMEM); >> } >> >> - pkt->data = (uint8_t *)wrapped; >> + av_frame_move_ref((AVFrame*)data, wrapped); >> + av_frame_free(&wrapped); > > I think this could be done as just > > av_frame_ref((AVFrame*)data, frame); > > without calling av_frame_clone(), but it doesn't hurt either. (Changing > it might be an optional, minor improvement.) You are right, yet I used av_frame_move_ref, because it overwrites the contents of AVFrame directly, so it does not matter that I allocated it with mallocz instead of av_frame_alloc, and av_frame_ref docs explicitly requires an unref-ed or alloc-ed AVFrame. So this seemed more safe, even if it involves a few more code lines. > >> + >> + pkt->data = data; >> pkt->size = sizeof(*wrapped); >> >> pkt->flags |= AV_PKT_FLAG_KEY; > > Patch looks good, and I like it better than checking the codec ID. Ok, will apply soon. Thanks, Marton
On Wed, 22 Feb 2017 21:00:31 +0100 (CET) Marton Balint <cus@passwd.hu> wrote: > On Wed, 22 Feb 2017, wm4 wrote: > > > On Wed, 22 Feb 2017 00:14:32 +0100 > > Marton Balint <cus@passwd.hu> wrote: > > > >> This ensures that the wrapped avframe will not get reallocated later, which > >> would invalidate internal references such as extended data. > >> > >> Signed-off-by: Marton Balint <cus@passwd.hu> > >> --- > >> libavcodec/wrapped_avframe.c | 16 ++++++++++++++-- > >> 1 file changed, 14 insertions(+), 2 deletions(-) > >> > >> diff --git a/libavcodec/wrapped_avframe.c b/libavcodec/wrapped_avframe.c > >> index 13c8d8a..1436032 100644 > >> --- a/libavcodec/wrapped_avframe.c > >> +++ b/libavcodec/wrapped_avframe.c > >> @@ -43,19 +43,31 @@ static int wrapped_avframe_encode(AVCodecContext *avctx, AVPacket *pkt, > >> const AVFrame *frame, int *got_packet) > >> { > >> AVFrame *wrapped = av_frame_clone(frame); > >> + uint8_t *data; > >> + int size = sizeof(*wrapped) + AV_INPUT_BUFFER_PADDING_SIZE; > >> > >> if (!wrapped) > >> return AVERROR(ENOMEM); > >> > >> - pkt->buf = av_buffer_create((uint8_t *)wrapped, sizeof(*wrapped), > >> + data = av_mallocz(size); > >> + if (!data) { > >> + av_frame_free(&wrapped); > >> + return AVERROR(ENOMEM); > >> + } > >> + > >> + pkt->buf = av_buffer_create(data, size, > >> wrapped_avframe_release_buffer, NULL, > >> AV_BUFFER_FLAG_READONLY); > >> if (!pkt->buf) { > >> av_frame_free(&wrapped); > >> + av_freep(&data); > >> return AVERROR(ENOMEM); > >> } > >> > >> - pkt->data = (uint8_t *)wrapped; > >> + av_frame_move_ref((AVFrame*)data, wrapped); > >> + av_frame_free(&wrapped); > > > > I think this could be done as just > > > > av_frame_ref((AVFrame*)data, frame); > > > > without calling av_frame_clone(), but it doesn't hurt either. (Changing > > it might be an optional, minor improvement.) > > You are right, yet I used av_frame_move_ref, because it overwrites the > contents of AVFrame directly, so it does not matter that I allocated it > with mallocz instead of av_frame_alloc, and av_frame_ref docs explicitly > requires an unref-ed or alloc-ed AVFrame. So this seemed more safe, even > if it involves a few more code lines. Shouldn't make a difference. I'm also just seeing that this violates ABI by using sizeof(AVFrame) (how typical of FFmpeg/Libav code to violate its own complicated ABI rules), but it was like this before and can't be fixed. Probably another thing that could be fixed with the next ABI bump. > > > >> + > >> + pkt->data = data; > >> pkt->size = sizeof(*wrapped); > >> > >> pkt->flags |= AV_PKT_FLAG_KEY; > > > > Patch looks good, and I like it better than checking the codec ID. > > Ok, will apply soon. OK
On Wed, 22 Feb 2017, wm4 wrote: >> > >> > Patch looks good, and I like it better than checking the codec ID. >> >> Ok, will apply soon. > > OK Thanks, pushed. Regards, Marton
diff --git a/libavcodec/wrapped_avframe.c b/libavcodec/wrapped_avframe.c index 13c8d8a..1436032 100644 --- a/libavcodec/wrapped_avframe.c +++ b/libavcodec/wrapped_avframe.c @@ -43,19 +43,31 @@ static int wrapped_avframe_encode(AVCodecContext *avctx, AVPacket *pkt, const AVFrame *frame, int *got_packet) { AVFrame *wrapped = av_frame_clone(frame); + uint8_t *data; + int size = sizeof(*wrapped) + AV_INPUT_BUFFER_PADDING_SIZE; if (!wrapped) return AVERROR(ENOMEM); - pkt->buf = av_buffer_create((uint8_t *)wrapped, sizeof(*wrapped), + data = av_mallocz(size); + if (!data) { + av_frame_free(&wrapped); + return AVERROR(ENOMEM); + } + + pkt->buf = av_buffer_create(data, size, wrapped_avframe_release_buffer, NULL, AV_BUFFER_FLAG_READONLY); if (!pkt->buf) { av_frame_free(&wrapped); + av_freep(&data); return AVERROR(ENOMEM); } - pkt->data = (uint8_t *)wrapped; + av_frame_move_ref((AVFrame*)data, wrapped); + av_frame_free(&wrapped); + + pkt->data = data; pkt->size = sizeof(*wrapped); pkt->flags |= AV_PKT_FLAG_KEY;
This ensures that the wrapped avframe will not get reallocated later, which would invalidate internal references such as extended data. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavcodec/wrapped_avframe.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)