[FFmpeg-devel] [PATCH] libavcodec/utils: Simplify get_buffer compatibility wrapper.
nfxjfg at googlemail.com
Mon Feb 17 00:13:39 CET 2014
On Mon, 17 Feb 2014 00:04:47 +0100
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> 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.
I think it does. The author of the reference counting API added this
code himself (see commit 7e350379f87e). Basically, giving out a
reference to some memory means you give access to all that memory.
> > 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?
If you want to preserve content in between the stride padding (like a
rectangular sub-area of a frame buffer), then yes, it breaks it. But
note that even "classic" APIs like libswscale like doing this. It's
convenient for API implementers, but not for users.
> > I agree that these semantics are a bit weird and confusing.
> They are breaking established functionality in that case.
Oops. Well, get_buffer is deprecated anyway.
> > > 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.
More information about the ffmpeg-devel