[FFmpeg-devel] [PATCH] libavcodec/utils: Simplify get_buffer compatibility wrapper.

[Reimar Döffinger]

On Mon, Feb 17, 2014 at 12:22:37AM +0100, wm4 wrote:
> On Mon, 17 Feb 2014 00:16:14 +0100
> Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> 
> > On Sun, Feb 16, 2014 at 11:57:23PM +0100, wm4 wrote:
> > > This is probably not legal. Code can assume that it can access the
> > > whole referenced buffer, not just where the plane data is. Look for
> > > example what vf_pad.c does: it tries to use "unused" space in the
> > > buffer data area to extend the planes without copy. But I'm not sure if
> > > I read this code right.
> > 
> > So by my current understanding, to _actually_ be backwards compatible,
> > the utils.c code would have to generate one refbuffer _per line_, not
> > per plane.
> > That is obviously quite insane...
> 
> Did the old API really guarantee not to write padding? I don't know
> much about the decoders, but usually ffmpeg likes stomping over padding
> so much, even very small data like extradata has FF_INPUT_PADDING.

The padding is basically what you get through align_dimensions.
If that returns something other than the size of the video then
there'd be some pixels you wouldn't know what might happen to them.
But especially with video sizes that are a multiple of 16 that is
fairly rare and then it should not be writing outside.
Though I doubt this was formally written down.
However I see several other issues:
1) av_frame_get_plane_buffer does not handle overlapping BufferRefs
correctly, it will just return whichever. The compatibility code can
generate overlapping BufferRefs. So these do not seem to agree on what
the API is.
2) Strictly speaking, IIRC all of av_frame_get_plane_buffer is undefined
behaviour. I would need to read up on the exact language, but comparing
pointers not from the same allocation area is undefined behaviour.
av_frame_get_plane_buffer not only compares pointers from completely
different allocations, it also assumes the result will be authoritative,
and coming back to 1) does not even do sanity-checking.

> Anyway, I think the emulation will work fine in most cases, as long as
> you don't pass refcounted frames to e.g. libavfilter - which you can't
> anyway, unless you explicitly enable libavcodec refcounting.

My "simplification" however seemed to work fine as well...
I am however investigating how to actually switch to refcounting
using code that won't break again in a few months.