[FFmpeg-devel,1/4] avcodec/mediacodecdec: clarify delay_flush specific code

Submitted by Aman Gupta on April 24, 2018, 8:59 p.m.

Details

Message ID 20180424205926.42027-1-ffmpeg@tmm1.net
State New
Headers show

Commit Message

Aman Gupta April 24, 2018, 8:59 p.m.
From: Aman Gupta <aman@tmm1.net>

---
 libavcodec/mediacodecdec.c        | 24 +++---------------------
 libavcodec/mediacodecdec_common.c | 12 ++++++++++++
 2 files changed, 15 insertions(+), 21 deletions(-)

Comments

Aman Gupta April 24, 2018, 9:14 p.m.
On Tue, Apr 24, 2018 at 2:11 PM, Jan Ekström <jeebjp@gmail.com> wrote:

> On Tue, Apr 24, 2018 at 11:59 PM, Aman Gupta <ffmpeg@tmm1.net> wrote:
> > From: Aman Gupta <aman@tmm1.net>
> >
> > ---
> >  libavcodec/mediacodecdec.c        | 24 +++---------------------
> >  libavcodec/mediacodecdec_common.c | 12 ++++++++++++
> >  2 files changed, 15 insertions(+), 21 deletions(-)
> >
> > diff --git a/libavcodec/mediacodecdec.c b/libavcodec/mediacodecdec.c
> > index 0d4a853f07..e5d3e6a0af 100644
> > --- a/libavcodec/mediacodecdec.c
> > +++ b/libavcodec/mediacodecdec.c
> > @@ -419,27 +419,9 @@ static int mediacodec_receive_frame(AVCodecContext
> *avctx, AVFrame *frame)
> >      MediaCodecH264DecContext *s = avctx->priv_data;
> >      int ret;
> >
> > -    /*
> > -     * MediaCodec.flush() discards both input and output buffers, thus
> we
> > -     * need to delay the call to this function until the user has
> released or
> > -     * renderered the frames he retains.
> > -     *
> > -     * After we have buffered an input packet, check if the codec is in
> the
> > -     * flushing state. If it is, we need to call
> ff_mediacodec_dec_flush.
> > -     *
> > -     * ff_mediacodec_dec_flush returns 0 if the flush cannot be
> performed on
> > -     * the codec (because the user retains frames). The codec stays in
> the
> > -     * flushing state.
> > -     *
> > -     * ff_mediacodec_dec_flush returns 1 if the flush can actually be
> > -     * performed on the codec. The codec leaves the flushing state and
> can
> > -     * process again packets.
> > -     *
> > -     * ff_mediacodec_dec_flush returns a negative value if an error has
> > -     * occurred.
> > -     *
> > -     */
> > -    if (ff_mediacodec_dec_is_flushing(avctx, s->ctx)) {
> > +    /* in delay_flush mode, wait until the user has released or rendered
> > +       all retained frames. */
> > +    if (s->delay_flush && ff_mediacodec_dec_is_flushing(avctx,
> s->ctx)) {
> >          if (!ff_mediacodec_dec_flush(avctx, s->ctx)) {
> >              return AVERROR(EAGAIN);
> >          }
>
> The commit message makes it sound as if you are only improving the
> documentation, while you have now added an additional limitation here
> (only check for flushing if delay_flush is true), so you might want to
> detail on the "What & Why" in the commit message. Otherwise looks
> alright.
>

There are no functional changes in this patch.

Previously, ff_mediacodec_dec_is_flushing() would only ever be true
if the user had set delay_flush. By adding that to the if statement, it
makes it clearer that this
part of the code is only in effect when that special option is set.

Aman


>
> Jan
>
Matthieu Bouron April 25, 2018, 4:04 p.m.
On Tue, Apr 24, 2018 at 01:59:23PM -0700, Aman Gupta wrote:
> From: Aman Gupta <aman@tmm1.net>
> 
> ---
>  libavcodec/mediacodecdec.c        | 24 +++---------------------
>  libavcodec/mediacodecdec_common.c | 12 ++++++++++++
>  2 files changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/libavcodec/mediacodecdec.c b/libavcodec/mediacodecdec.c
> index 0d4a853f07..e5d3e6a0af 100644
> --- a/libavcodec/mediacodecdec.c
> +++ b/libavcodec/mediacodecdec.c
> @@ -419,27 +419,9 @@ static int mediacodec_receive_frame(AVCodecContext *avctx, AVFrame *frame)
>      MediaCodecH264DecContext *s = avctx->priv_data;
>      int ret;
>  
> -    /*
> -     * MediaCodec.flush() discards both input and output buffers, thus we
> -     * need to delay the call to this function until the user has released or
> -     * renderered the frames he retains.
> -     *
> -     * After we have buffered an input packet, check if the codec is in the
> -     * flushing state. If it is, we need to call ff_mediacodec_dec_flush.
> -     *
> -     * ff_mediacodec_dec_flush returns 0 if the flush cannot be performed on
> -     * the codec (because the user retains frames). The codec stays in the
> -     * flushing state.
> -     *
> -     * ff_mediacodec_dec_flush returns 1 if the flush can actually be
> -     * performed on the codec. The codec leaves the flushing state and can
> -     * process again packets.
> -     *
> -     * ff_mediacodec_dec_flush returns a negative value if an error has
> -     * occurred.
> -     *
> -     */
> -    if (ff_mediacodec_dec_is_flushing(avctx, s->ctx)) {
> +    /* in delay_flush mode, wait until the user has released or rendered

NIT: In delay_flush mode, [...]

> +       all retained frames. */
> +    if (s->delay_flush && ff_mediacodec_dec_is_flushing(avctx, s->ctx)) {
>          if (!ff_mediacodec_dec_flush(avctx, s->ctx)) {
>              return AVERROR(EAGAIN);
>          }
> diff --git a/libavcodec/mediacodecdec_common.c b/libavcodec/mediacodecdec_common.c
> index e59cf19aad..0c27624dea 100644
> --- a/libavcodec/mediacodecdec_common.c
> +++ b/libavcodec/mediacodecdec_common.c
> @@ -764,6 +764,18 @@ int ff_mediacodec_dec_receive(AVCodecContext *avctx, MediaCodecDecContext *s,
>      return AVERROR(EAGAIN);
>  }
>  
> +/*
> +* ff_mediacodec_dec_flush returns 0 if the flush cannot be performed on
> +* the codec (because the user retains frames). The codec stays in the
> +* flushing state.
> +*
> +* ff_mediacodec_dec_flush returns 1 if the flush can actually be
> +* performed on the codec. The codec leaves the flushing state and can
> +* process again packets.
> +*
> +* ff_mediacodec_dec_flush returns a negative value if an error has
> +* occurred.
> +*/
>  int ff_mediacodec_dec_flush(AVCodecContext *avctx, MediaCodecDecContext *s)
>  {
>      if (!s->surface || atomic_load(&s->refcount) == 1) {
> -- 
> 2.14.2
> 

LGTM.

Patch hide | download patch | download mbox

diff --git a/libavcodec/mediacodecdec.c b/libavcodec/mediacodecdec.c
index 0d4a853f07..e5d3e6a0af 100644
--- a/libavcodec/mediacodecdec.c
+++ b/libavcodec/mediacodecdec.c
@@ -419,27 +419,9 @@  static int mediacodec_receive_frame(AVCodecContext *avctx, AVFrame *frame)
     MediaCodecH264DecContext *s = avctx->priv_data;
     int ret;
 
-    /*
-     * MediaCodec.flush() discards both input and output buffers, thus we
-     * need to delay the call to this function until the user has released or
-     * renderered the frames he retains.
-     *
-     * After we have buffered an input packet, check if the codec is in the
-     * flushing state. If it is, we need to call ff_mediacodec_dec_flush.
-     *
-     * ff_mediacodec_dec_flush returns 0 if the flush cannot be performed on
-     * the codec (because the user retains frames). The codec stays in the
-     * flushing state.
-     *
-     * ff_mediacodec_dec_flush returns 1 if the flush can actually be
-     * performed on the codec. The codec leaves the flushing state and can
-     * process again packets.
-     *
-     * ff_mediacodec_dec_flush returns a negative value if an error has
-     * occurred.
-     *
-     */
-    if (ff_mediacodec_dec_is_flushing(avctx, s->ctx)) {
+    /* in delay_flush mode, wait until the user has released or rendered
+       all retained frames. */
+    if (s->delay_flush && ff_mediacodec_dec_is_flushing(avctx, s->ctx)) {
         if (!ff_mediacodec_dec_flush(avctx, s->ctx)) {
             return AVERROR(EAGAIN);
         }
diff --git a/libavcodec/mediacodecdec_common.c b/libavcodec/mediacodecdec_common.c
index e59cf19aad..0c27624dea 100644
--- a/libavcodec/mediacodecdec_common.c
+++ b/libavcodec/mediacodecdec_common.c
@@ -764,6 +764,18 @@  int ff_mediacodec_dec_receive(AVCodecContext *avctx, MediaCodecDecContext *s,
     return AVERROR(EAGAIN);
 }
 
+/*
+* ff_mediacodec_dec_flush returns 0 if the flush cannot be performed on
+* the codec (because the user retains frames). The codec stays in the
+* flushing state.
+*
+* ff_mediacodec_dec_flush returns 1 if the flush can actually be
+* performed on the codec. The codec leaves the flushing state and can
+* process again packets.
+*
+* ff_mediacodec_dec_flush returns a negative value if an error has
+* occurred.
+*/
 int ff_mediacodec_dec_flush(AVCodecContext *avctx, MediaCodecDecContext *s)
 {
     if (!s->surface || atomic_load(&s->refcount) == 1) {