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

[wm4]

On Mon, 17 Feb 2014 00:54:09 +0100
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> 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.

In addition, I don't think this code really helps with anything. I
think it was made for the case to avoid image reallocation when you crop
an image, and than pad it again. But this really seems like an obscure
case, and isn't worth all this trouble.

So I would also prefer if buffer references don't really reference
plane memory, but are merely "abstract" references for the sake of
refcounting.

> > 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.