On Thu, 14 Apr 2016 14:19:42 +0200 Matthieu Bouron <matthieu.bou...@gmail.com> wrote:
> On Thu, Apr 7, 2016 at 4:18 PM, wm4 <nfx...@googlemail.com> wrote: > > > On Fri, 18 Mar 2016 17:50:39 +0100 > > Matthieu Bouron <matthieu.bou...@gmail.com> wrote: > > > > > From: Matthieu Bouron <matthieu.bou...@stupeflix.com> > > > > > > --- > > > > > > Hello, > > > > Can't say much about this, so just some minor confused comments. > > > > Thanks for your comments and sorry for the late reply. Well, I've also taken some time to send that review... > > > > > > > > > The following patch add hwaccel support to the mediacodec (h264) decoder > > by allowing > > > the user to render the output frames directly on a surface. > > > > > > In order to do so the user needs to initialize the hwaccel through the > > use of > > > av_mediacodec_alloc_context and av_mediacodec_default_init functions. > > The later > > > takes a reference to an android/view/Surface as parameter. > > > > > > If the hwaccel successfully initialize, the decoder output frames pix > > fmt will be > > > AV_PIX_FMT_MEDIACODEC. The following snippet of code demonstrate how to > > render > > > the frames on the surface: > > > > > > AVMediaCodecBuffer *buffer = (AVMediaCodecBuffer *)frame->data[3]; > > > av_mediacodec_release_buffer(buffer, 1); > > > > > > The last argument of av_mediacodec_release_buffer enable rendering of the > > > buffer on the surface (or not if set to 0). > > > > > > > I don't understand this (at all), but unreferencing the AVFrame should > > unref the underlying surface. > > > > In this case, the underlying surface will remain (it is owned by the codec > itself) but the output buffer (that should be renderered to the surface) > will be discarded. > So: the AVFrame (and AVMediaCodecBuffer) really reference two buffers, the codec and output buffer? And this API call releases the codec buffer? Why can't it be released immediately? > > > > > Regarding the internal changes in the mediacodec decoder: > > > > > > MediaCodec.flush() discards both input and output buffers meaning that if > > > MediaCodec.flush() is called all output buffers the user has a reference > > on are > > > now invalid (and cannot be used). > > > This behaviour does not fit well in the avcodec API. > > > > > > When the decoder is configured to output software buffers, there is no > > issue as > > > the buffers are copied. > > > > > > Now when the decoder is configured to output to a surface, the user > > might not > > > want to render all the frames as fast as the decoder can go and might > > want to > > > control *when* the frame are rendered, so we need to make sure that the > > > MediaCodec.flush() call is delayed until all the frames the user retains > > has > > > been released or rendered. > > > > > > Delaying the call to MediaCodec.flush() means buffering any inputs that > > come > > > the decoder until the user has released/renderer the frame he retains. > > > > > > This is a limitation of this hwaccel implementation, if the user retains > > a > > > frame (a), then issue a flush command to the decoder, the packets he > > feeds to > > > the decoder at that point will be queued in the internal decoder packet > > queue > > > (until he releases the frame (a)). This scenario leads to a memory usage > > > increase to say the least. > > > > > > Currently there is no limitation on the size of the internal decoder > > packet > > > queue but this is something that can be added easily. Then, if the queue > > is > > > full, what would be the behaviour of the decoder ? Can it block ? Or > > should it > > > returns something like AVERROR(EAGAIN) ? > > > > The current API can't do anything like this. It has to output 0 or 1 > > frame per input packet. (If it outputs nothing, the frame is either > > discarded or queued internally. The queue can be emptied only when > > draining the decoder at the end of the stream.) > > > > So it looks like all you can do is blocking. (Which could lead to a > > deadlock in the API user, depending of how the user's code works?) > > > > Yes if I block at some point, it can lead to a deadlock if the user never > releases all the frames. I'm considering buffering a few input packets > before blocking. OK, I guess this can't be avoided. Maybe the user should get the possibility to control how many surfaces are buffered at most (before a deadlock happens). > > > > > > > > > About the other internal decoder changes I introduced: > > > > > > The MediaCodecDecContext is now refcounted (using the lavu/atomic api) > > since > > > the (hwaccel) frames can be retained by the user, we need to delay the > > > destruction of the codec until the user has released all the frames he > > has a > > > reference on. > > > The reference counter of the MediaCodecDecContext is incremented each > > time an > > > (hwaccel) frame is outputted by the decoder and decremented each time a > > > (hwaccel) frame is released. > > > > > > Also, when the decoder is configured to output to a surface the pts that > > are > > > given to the MediaCodec API are now rescaled based on the codec_timebase > > as > > > those timestamps values are propagated to the frames rendered on the > > surface > > > since Android M. Not sure if it's really useful though. > > > > That's all nice, but where is this stuff documented at all? > > > > It is documented here: > http://developer.android.com/reference/android/media/MediaCodec.html#queueInputBuffer%28int,%20int,%20int,%20long,%20int%29 OK, I was just thinking that there might be things specific to the ffmpeg wrapper. > > > + > > > +static int mediacodec_dec_flush_codec(AVCodecContext *avctx, > > MediaCodecDecContext *s) > > > +{ > > > + FFAMediaCodec *codec = s->codec; > > > + int status; > > > + > > > + s->queued_buffer_nb = 0; > > > + s->dequeued_buffer_nb = 0; > > > + > > > + s->draining = 0; > > > + s->flushing = 0; > > > + > > > + status = ff_AMediaCodec_flush(codec); > > > + if (status < 0) { > > > + av_log(NULL, AV_LOG_ERROR, "Failed to flush MediaCodec %p", > > codec); > > > + return AVERROR_EXTERNAL; > > > + } > > > + > > > + s->first_buffer = 0; > > > + s->first_buffer_at = av_gettime(); > > > > What is the system time doing here? > > > > It is here for debugging/information purpose, to know how many time it took > to the codec to buffer and output its first buffer when it starts or after > a flush (discard). Well, a bit questionable to have in the final code, but ok I guess. > > I will wait the hwaccel stuff from libav to be merged in order to resubmit > (and push if ok) this patch. I don't think this changes much. There is now AVHWFramesContext, which may help with refcounting some things and simplifying the opaque/readback code paths. I find it useful, but it's not an absolute requirement. The old API still works, both internally and externally. Also, most of these changes have been already merged for a while. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
