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

[Marton Balint]

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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[wm4]

On Wed, 22 Feb 2017 21:00:31 +0100 (CET)
Marton Balint  wrote:

> On Wed, 22 Feb 2017, wm4 wrote:
> 
> > On Wed, 22 Feb 2017 00:14:32 +0100
> > Marton Balint  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 
> >> ---
> >>  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();
> >> +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();
> >> +av_freep();
> >>  return AVERROR(ENOMEM);
> >>  }
> >> 
> >> -pkt->data = (uint8_t *)wrapped;
> >> +av_frame_move_ref((AVFrame*)data, wrapped);
> >> +av_frame_free();  
> >
> > 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Marton Balint]

On Wed, 22 Feb 2017, wm4 wrote:


On Wed, 22 Feb 2017 00:14:32 +0100
Marton Balint  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 
---
 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();
+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();
+av_freep();
 return AVERROR(ENOMEM);
 }

-pkt->data = (uint8_t *)wrapped;
+av_frame_move_ref((AVFrame*)data, wrapped);
+av_frame_free();


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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[wm4]

On Wed, 22 Feb 2017 00:14:32 +0100
Marton Balint  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 
> ---
>  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();
> +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();
> +av_freep();
>  return AVERROR(ENOMEM);
>  }
>  
> -pkt->data = (uint8_t *)wrapped;
> +av_frame_move_ref((AVFrame*)data, wrapped);
> +av_frame_free();

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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Marton Balint]

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 
---
 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();
+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();
+av_freep();
 return AVERROR(ENOMEM);
 }
 
-pkt->data = (uint8_t *)wrapped;
+av_frame_move_ref((AVFrame*)data, wrapped);
+av_frame_free();
+
+pkt->data = data;
 pkt->size = sizeof(*wrapped);
 
 pkt->flags |= AV_PKT_FLAG_KEY;
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel