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

[Reimar Döffinger]

On Sun, Feb 16, 2014 at 11:57:23PM +0100, wm4 wrote:
> On Sun, 16 Feb 2014 23:43:44 +0100
> Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> 
> > On Sun, Feb 16, 2014 at 11:34:23PM +0100, Reimar Döffinger wrote:
> > > On Sun, Feb 16, 2014 at 11:17:05PM +0100, wm4 wrote:
> > > > The assumption of the AVFrame API is that all memory ranges referenced
> > > > are covered by buffer refs. You seem to think that it's ok to have
> > > > merely an abstract reference for the entire frame, but apparently this
> > > > is not enough. The code you remove is an attempt to account for all
> > > > data correctly. Since you don't know whether all the planes are
> > > > allocated with a single memory allocation or whether they're separate
> > > > allocations, you have to assume the worst case, and use a buffer per
> > > > plane.
> > > 
> > > The problem is the only thing in the documentation hinting at this is:
> > > > every single data plane must be contained in one of the buffers
> > > 
> > > However a single buffer starting at address 0 and going all the way up
> > > to the highest addressable address would fulfill this, which is
> > > almost what my patch does.
> 
> 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.

I don't think this is legal. In a user-allocated buffer there is nothing
guaranteeing that any of this area is valid to use (it might be just
a window inside a framebuffer, the allocated area is the whole
framebuffer but touching anything outside the lines will destroy the
framebuffer content), and the API does not require that all memory
in the buffers is ok to access.

> Basically, forget about buffer refs as abstract refcounting: they
> really refer memory, and code is free to make use of all memory that is
> covered by a reference. (Of course, for hwaccel it's all different
> again!)

So the refcounted frames were in fact a backdoor way of breaking
user-provided buffers?

> I agree that these semantics are a bit weird and confusing.

They are breaking established functionality in that case.

> > Because either the "if" part
> > a) is a completely obvious and stupid no-op, but I can't understand how this was missed, in multiple places to top it off
> > b) isn't a no-op and I don't understand how I can't see it
> 
> It's not a no-op. AVFrame was probably designed for video data, but
> then someone came and thought it was a good idea to reuse it for audio.
> Video has only 4 planes max, but audio (ever since "planar audio" was
> invented) can have much more frames. Since the plane array was fixed
> size, and someone wanted to avoid having each AVFrame user allocate the
> data array in the video case, extended_data was introduced to handle
> the needs of audio in case there are more channels than the data array
> can handle. The ffmpeg API is fun.

Yeah, I just missed the array vs. pointer thing, I guess this
is a case where C making this transparent can really hurt readability.
At least for me.