[FFmpeg-devel] [PATCH] avcodec/wrapped_avframe: allocate a buffer with padding

[wm4]

On Wed, 22 Feb 2017 21:00:31 +0100 (CET)
Marton Balint <cus at passwd.hu> wrote:

> On Wed, 22 Feb 2017, wm4 wrote:
> 
> > On Wed, 22 Feb 2017 00:14:32 +0100
> > Marton Balint <cus at 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 at 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