On Tue, 29 Aug 2017 12:03:42 +0200 Jorge Ramirez <jorge.ramirez-or...@linaro.org> wrote:
> On 08/29/2017 10:56 AM, wm4 wrote: > > On Mon, 28 Aug 2017 23:36:08 +0200 > > Jorge Ramirez <jorge.ramirez-or...@linaro.org> wrote: > > > >> On 08/28/2017 09:53 PM, wm4 wrote: > >>> On Mon, 28 Aug 2017 21:24:26 +0200 > >>> Jorge Ramirez <jorge.ramirez-or...@linaro.org> wrote: > >>> > >>>> On 08/28/2017 02:16 PM, Jorge Ramirez wrote: > >>>>> On 08/28/2017 12:47 PM, wm4 wrote: > >>>>>>> I guess that instead of polling for the AVBufferRef to be > >>>>>>> unreferenced, > >>>>>>> I can associate a sync (ie a sempahore) to each buffer, take it on > >>>>>>> release and post the semaphore on the AVBufferRefs being unreferenced. > >>>>>>> that is actually pretty clean in terms of cpu usage. > >>>>>> That would just freeze an API user calling avcodec_close(), when it > >>>>>> keeps around decoded AVFrames for later use. > >>>>> yes I understand, but it does avoid using the CPU to poll for the > >>>>> buffer release (an incremental improvement) > >>>>> > >>>>> but yes I think that the message is that even though this proposal > >>>>> might suffice for simple video players (my tests) is not good enough > >>>>> for other users requiring the decoded frame for post processing. > >>>>> > >>>>> is this a blocker to upstream or could I continue working with it > >>>>> flagging the encoder/decoder as EXPERIMENTAL? the current situation at > >>>>> least keeps video players happy. > >>> I'd say yes this is a blocker. We usually try to avoid committing > >>> half-finished code, because it often means it will be never finished. > >> hi, I forgot to say earlier, thanks for all the review over the past > >> couple of days (it has been of much help). > >> > >> on the half finished matter, the issue that I face is that the current > >> code doesn't cover the use case where _all_ the processed frames have to > >> be kept available indefinitely (this is why I thought that perhaps > >> setting .capabilities to AV_CODEC_CAP_EXPERIMENTAL could be an option to > >> upstream and get more exposure to other users; > >> > >> I do plan to continue supporting v4l2 ffmpeg integration - mmaped > >> filters, DRM and so on...having invested this long I do want to see this > >> through; and since I can't guaranteed that some "force majeure" wont > >> happen I think the sooner the code I have been working on can get > >> exposure the sooner we will start seeing contributions. > >> > >> Anyhow, the current code does support the typical use case of most video > >> players so it would benefit a considerable amount of users. > >> > >> does it have to be an all or nothing at this point or could we flag the > >> v4l2 m2m as experimental codecs? > > You could just copy the frames before returning them to the user to > > avoid breaking refcounting. > > thinking again about this I'd rather not do that (it will impact > performance too much) and Hendrik gave me some pointers yesterday in > line with what you said as well. > I implemented reference counting delegating the closing of _some_ > resources needed to keep the buffers alive. > > closing the codec now doesnt wait or leave dangling buffers. > > the AVBufferRef free callback looks just like this > > static void free_v4l2buf_cb(void *opaque, uint8_t *unused) > { > V4L2Buffer* avbuf = opaque; > V4L2m2mContext *s = container_of(avbuf->context, V4L2m2mContext, > capture); > > atomic_fetch_sub_explicit(&s->refcount, 1, memory_order_acq_rel); > > if (s->reinit) { > if (!atomic_load(&s->refcount)) > sem_post(&s->refsync); > return; > } > > if (avbuf->context->streamon) { > avbuf->context->ops.enqueue(avbuf); > return; > } > > if (!atomic_load(&s->refcount)) > avpriv_v4l2m2m_end(s); > } > > The only case where I can't get away without waiting for the AVBufferRef > to be released is when re-initializing the frame dimensions (ie, > resolution changes/format) _during_ streaming since I need to release > _all_ hardware buffers and queue them again. > > will this be acceptable? > I have just tested these changes and works as expected. The implementation seems rather roundabout and complex - why not use AVBufferRef? But apart from that, yes. > > > >>> > >>>>> > >>>> just wondering, if the AVBufferRefs must live for ever (ie, after the > >>>> codecs have been closed), what do other codecs dequeuing from a limited > >>>> number of re-usable hardware allocated buffers do? > >>>> do they use the CPU allocate and copy the data from those buffers to the > >>>> heap? > >>>> > >>> Like I wrote before: hwaccels use AVHWFramesContext, which was made > >>> more or less for this situation. If you want FD support later (for > >>> something like zero-copy transcoding or playback), AVHWFramesContext > >>> will probably be mandatory anyway. But I guess it's a big change for > >>> someone not familiar with the codebase. > >> Yes I had a look and it seems not an easy change to integrate. > >> > >> Still I'd like to make sure we are talking about the same requirement > >> because if AVHWFramesContext works around the issue [1] , I can probably > >> do the same with a few more lines of code (including the FD support for > >> v4l2 which is pretty straight forward) > >> > >> [1] When: > >> a) there is a limited number of buffers allocated by the hardware and > >> b) these buffers are mapped to the process address space and > >> c) the application can choose to keep _all_ decoded buffers for post > >> processing, > >> > >> then there is no other option than copying each of the processed buffers > >> to newly allocated areas in the heap (there can be no magic on this > >> since the hardware buffers are always limited and have to be reused). > > The semantics of AVHWFramesContext are such that if the user keeps > > enough AVFrames references to exhaust the frame pool, trying to continue > > decoding will result in an error. The whole point is to make the > > limited and fixed buffer allocation visible to the API user. > > makes sense although I havent found such an interface; in my view, the > user should be able to register an observer to receive async events from > codecs (be these from hardware or codec state machines) > could you point me where that is? the way I understand ffmpeg is that > everything seem to be working synchronously with no room for events like > this (so the user would only be reported of an error after it tries to > get a frame for instance..) that doesn't seem to have anything to do with our previous discussion. Async behavior could be easily added to the API. But the current API is a state machine that doesn't know about time. > >> notice that I do insist on continue using V4L2_MEMORY_MMAP (instead of > >> switching to V4L2_MEMORY_USERPTR) because it is easy to export the > >> buffers as DMABUFs (~30 lines of code) and then pass these in FDs (which > >> could be associated to short lived AVBufferRefs for DRM) > > No idea what's the difference between those. > > > > If you want to support direct FD/dmabuf export, adapting to > > AVHWFramesContext now would probably be easier in total. Especially > > because of the implied API change. But I'd wait for Mark Thompson's > > comments on that before making any big changes. AFAIK he posted a > > proposal patch for a DRM AVHWFramesContext too. > > why not just add a FD to the AVBufferRef and let the user decide whether > to use it or not? Because there's too much framework-y stuff in ffmpeg that expects AVHWFramesContext. It's basically needed to do anything with it, and avoids ad-hoc approaches the old hwaccel API was full of. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel