[FFmpeg-devel] lavc/hevcdec: HEVC decoder getting format clean up

Submitted by Jun Zhao on Oct. 24, 2018, 3:05 p.m.

Details

Message ID 1540393553-10656-1-git-send-email-mypopydev@gmail.com
State New
Headers show

Commit Message

Jun Zhao Oct. 24, 2018, 3:05 p.m.
From: Jun Zhao <jun.zhao@intel.com>

Add checking to avoid calling ff_thread_get_format repeatedly
whenever new slice header decoded.

Signed-off-by: Lin Xie <lin.xie@intel.com>
Signed-off-by: Jun Zhao <jun.zhao@intel.com>
---
 libavcodec/hevcdec.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

Comments

Hendrik Leppkes Oct. 24, 2018, 3:09 p.m.
On Wed, Oct 24, 2018 at 5:06 PM Jun Zhao <mypopydev@gmail.com> wrote:
>
> From: Jun Zhao <jun.zhao@intel.com>
>
> Add checking to avoid calling ff_thread_get_format repeatedly
> whenever new slice header decoded.
>
> Signed-off-by: Lin Xie <lin.xie@intel.com>
> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
> ---
>  libavcodec/hevcdec.c |   17 +++++++++++------
>  1 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> index a3b5c8c..16032a5 100644
> --- a/libavcodec/hevcdec.c
> +++ b/libavcodec/hevcdec.c
> @@ -317,7 +317,6 @@ static void export_stream_params(AVCodecContext *avctx, const HEVCParamSets *ps,
>      const HEVCWindow *ow = &sps->output_window;
>      unsigned int num = 0, den = 0;
>
> -    avctx->pix_fmt             = sps->pix_fmt;
>      avctx->coded_width         = sps->width;
>      avctx->coded_height        = sps->height;
>      avctx->width               = sps->width  - ow->left_offset - ow->right_offset;
> @@ -357,7 +356,7 @@ static void export_stream_params(AVCodecContext *avctx, const HEVCParamSets *ps,
>                    num, den, 1 << 30);
>  }
>
> -static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps)
> +static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps, int force_callback)
>  {
>  #define HWACCEL_MAX (CONFIG_HEVC_DXVA2_HWACCEL + \
>                       CONFIG_HEVC_D3D11VA_HWACCEL * 2 + \
> @@ -366,6 +365,8 @@ static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps)
>                       CONFIG_HEVC_VIDEOTOOLBOX_HWACCEL + \
>                       CONFIG_HEVC_VDPAU_HWACCEL)
>      enum AVPixelFormat pix_fmts[HWACCEL_MAX + 2], *fmt = pix_fmts;
> +    const enum AVPixelFormat *choices = pix_fmts;
> +    int i;
>
>      switch (sps->pix_fmt) {
>      case AV_PIX_FMT_YUV420P:
> @@ -418,6 +419,11 @@ static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps)
>      *fmt++ = sps->pix_fmt;
>      *fmt = AV_PIX_FMT_NONE;
>
> +    if (!force_callback)
> +        for (i = 0; choices[i] != AV_PIX_FMT_NONE; i++)
> +            if (choices[i] == s->avctx->pix_fmt)
> +                return choices[i];
> +
>      return ff_thread_get_format(s->avctx, pix_fmts);
>  }
>
> @@ -439,8 +445,6 @@ static int set_sps(HEVCContext *s, const HEVCSPS *sps,
>
>      export_stream_params(s->avctx, &s->ps, sps);
>
> -    s->avctx->pix_fmt = pix_fmt;
> -
>      ff_hevc_pred_init(&s->hpc,     sps->bit_depth);
>      ff_hevc_dsp_init (&s->hevcdsp, sps->bit_depth);
>      ff_videodsp_init (&s->vdsp,    sps->bit_depth);
> @@ -526,10 +530,11 @@ static int hls_slice_header(HEVCContext *s)
>          if (ret < 0)
>              return ret;
>
> -        pix_fmt = get_format(s, sps);
> +        pix_fmt = get_format(s, sps, s->avctx->hwaccel ? 0 : 1);

This seems rather wonky to me. Either there is a format change or
there isn't, which should be the deciding factor if get_format is
being called, not if a hwaccel is already on or not.

- Hendrik
Hendrik Leppkes Oct. 24, 2018, 3:22 p.m.
On Wed, Oct 24, 2018 at 5:09 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
> On Wed, Oct 24, 2018 at 5:06 PM Jun Zhao <mypopydev@gmail.com> wrote:
> >
> > From: Jun Zhao <jun.zhao@intel.com>
> >
> > Add checking to avoid calling ff_thread_get_format repeatedly
> > whenever new slice header decoded.
> >
> > Signed-off-by: Lin Xie <lin.xie@intel.com>
> > Signed-off-by: Jun Zhao <jun.zhao@intel.com>
> > ---
> >  libavcodec/hevcdec.c |   17 +++++++++++------
> >  1 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> > index a3b5c8c..16032a5 100644
> > --- a/libavcodec/hevcdec.c
> > +++ b/libavcodec/hevcdec.c
> > @@ -317,7 +317,6 @@ static void export_stream_params(AVCodecContext *avctx, const HEVCParamSets *ps,
> >      const HEVCWindow *ow = &sps->output_window;
> >      unsigned int num = 0, den = 0;
> >
> > -    avctx->pix_fmt             = sps->pix_fmt;
> >      avctx->coded_width         = sps->width;
> >      avctx->coded_height        = sps->height;
> >      avctx->width               = sps->width  - ow->left_offset - ow->right_offset;
> > @@ -357,7 +356,7 @@ static void export_stream_params(AVCodecContext *avctx, const HEVCParamSets *ps,
> >                    num, den, 1 << 30);
> >  }
> >
> > -static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps)
> > +static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps, int force_callback)
> >  {
> >  #define HWACCEL_MAX (CONFIG_HEVC_DXVA2_HWACCEL + \
> >                       CONFIG_HEVC_D3D11VA_HWACCEL * 2 + \
> > @@ -366,6 +365,8 @@ static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps)
> >                       CONFIG_HEVC_VIDEOTOOLBOX_HWACCEL + \
> >                       CONFIG_HEVC_VDPAU_HWACCEL)
> >      enum AVPixelFormat pix_fmts[HWACCEL_MAX + 2], *fmt = pix_fmts;
> > +    const enum AVPixelFormat *choices = pix_fmts;
> > +    int i;
> >
> >      switch (sps->pix_fmt) {
> >      case AV_PIX_FMT_YUV420P:
> > @@ -418,6 +419,11 @@ static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps)
> >      *fmt++ = sps->pix_fmt;
> >      *fmt = AV_PIX_FMT_NONE;
> >
> > +    if (!force_callback)
> > +        for (i = 0; choices[i] != AV_PIX_FMT_NONE; i++)
> > +            if (choices[i] == s->avctx->pix_fmt)
> > +                return choices[i];
> > +

Additionally, there can be a lot more properties that might result in
a different hwaccel selection, or even software fallback. Resolution,
frame rate, who knows what else.
The point of get_format is not only to determine the frame format, but
also notify the usercode / hwaccel about any changes in stream
properties.

If too many calls to get_format cause serious issues, then typically
the usercode that implements get_format is written badly. It should
verify that the current hwaccel is still capable of decoding the new
format, and only then act, instead of always re-creating the hardware
decoder. I realize that our avcodec code is also guilty of that, but
I've complained about that before, and thats the prime reason I also
don't use it. :)

- Hendrik
mypopy@gmail.com Oct. 25, 2018, 6:49 a.m.
On Wed, Oct 24, 2018 at 11:10 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
> On Wed, Oct 24, 2018 at 5:06 PM Jun Zhao <mypopydev@gmail.com> wrote:
> >
> > From: Jun Zhao <jun.zhao@intel.com>
> >
> > Add checking to avoid calling ff_thread_get_format repeatedly
> > whenever new slice header decoded.
> >
> > Signed-off-by: Lin Xie <lin.xie@intel.com>
> > Signed-off-by: Jun Zhao <jun.zhao@intel.com>
> > ---
> >  libavcodec/hevcdec.c |   17 +++++++++++------
> >  1 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> > index a3b5c8c..16032a5 100644
> > --- a/libavcodec/hevcdec.c
> > +++ b/libavcodec/hevcdec.c
> > @@ -317,7 +317,6 @@ static void export_stream_params(AVCodecContext *avctx, const HEVCParamSets *ps,
> >      const HEVCWindow *ow = &sps->output_window;
> >      unsigned int num = 0, den = 0;
> >
> > -    avctx->pix_fmt             = sps->pix_fmt;
> >      avctx->coded_width         = sps->width;
> >      avctx->coded_height        = sps->height;
> >      avctx->width               = sps->width  - ow->left_offset - ow->right_offset;
> > @@ -357,7 +356,7 @@ static void export_stream_params(AVCodecContext *avctx, const HEVCParamSets *ps,
> >                    num, den, 1 << 30);
> >  }
> >
> > -static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps)
> > +static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps, int force_callback)
> >  {
> >  #define HWACCEL_MAX (CONFIG_HEVC_DXVA2_HWACCEL + \
> >                       CONFIG_HEVC_D3D11VA_HWACCEL * 2 + \
> > @@ -366,6 +365,8 @@ static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps)
> >                       CONFIG_HEVC_VIDEOTOOLBOX_HWACCEL + \
> >                       CONFIG_HEVC_VDPAU_HWACCEL)
> >      enum AVPixelFormat pix_fmts[HWACCEL_MAX + 2], *fmt = pix_fmts;
> > +    const enum AVPixelFormat *choices = pix_fmts;
> > +    int i;
> >
> >      switch (sps->pix_fmt) {
> >      case AV_PIX_FMT_YUV420P:
> > @@ -418,6 +419,11 @@ static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps)
> >      *fmt++ = sps->pix_fmt;
> >      *fmt = AV_PIX_FMT_NONE;
> >
> > +    if (!force_callback)
> > +        for (i = 0; choices[i] != AV_PIX_FMT_NONE; i++)
> > +            if (choices[i] == s->avctx->pix_fmt)
> > +                return choices[i];
> > +
> >      return ff_thread_get_format(s->avctx, pix_fmts);
> >  }
> >
> > @@ -439,8 +445,6 @@ static int set_sps(HEVCContext *s, const HEVCSPS *sps,
> >
> >      export_stream_params(s->avctx, &s->ps, sps);
> >
> > -    s->avctx->pix_fmt = pix_fmt;
> > -
> >      ff_hevc_pred_init(&s->hpc,     sps->bit_depth);
> >      ff_hevc_dsp_init (&s->hevcdsp, sps->bit_depth);
> >      ff_videodsp_init (&s->vdsp,    sps->bit_depth);
> > @@ -526,10 +530,11 @@ static int hls_slice_header(HEVCContext *s)
> >          if (ret < 0)
> >              return ret;
> >
> > -        pix_fmt = get_format(s, sps);
> > +        pix_fmt = get_format(s, sps, s->avctx->hwaccel ? 0 : 1);
>
> This seems rather wonky to me. Either there is a format change or
> there isn't, which should be the deciding factor if get_format is
> being called, not if a hwaccel is already on or not.
>
I know the concern now, in fact, this patch wants to reduce the call
number for hwaccel_uninit/hwaccel_init (in ff_get_format) again and
again , so use the hwaccel as flag in this case. Maybe we need to
found a more suitable in this case.

Patch hide | download patch | download mbox

diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index a3b5c8c..16032a5 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -317,7 +317,6 @@  static void export_stream_params(AVCodecContext *avctx, const HEVCParamSets *ps,
     const HEVCWindow *ow = &sps->output_window;
     unsigned int num = 0, den = 0;
 
-    avctx->pix_fmt             = sps->pix_fmt;
     avctx->coded_width         = sps->width;
     avctx->coded_height        = sps->height;
     avctx->width               = sps->width  - ow->left_offset - ow->right_offset;
@@ -357,7 +356,7 @@  static void export_stream_params(AVCodecContext *avctx, const HEVCParamSets *ps,
                   num, den, 1 << 30);
 }
 
-static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps)
+static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps, int force_callback)
 {
 #define HWACCEL_MAX (CONFIG_HEVC_DXVA2_HWACCEL + \
                      CONFIG_HEVC_D3D11VA_HWACCEL * 2 + \
@@ -366,6 +365,8 @@  static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps)
                      CONFIG_HEVC_VIDEOTOOLBOX_HWACCEL + \
                      CONFIG_HEVC_VDPAU_HWACCEL)
     enum AVPixelFormat pix_fmts[HWACCEL_MAX + 2], *fmt = pix_fmts;
+    const enum AVPixelFormat *choices = pix_fmts;
+    int i;
 
     switch (sps->pix_fmt) {
     case AV_PIX_FMT_YUV420P:
@@ -418,6 +419,11 @@  static enum AVPixelFormat get_format(HEVCContext *s, const HEVCSPS *sps)
     *fmt++ = sps->pix_fmt;
     *fmt = AV_PIX_FMT_NONE;
 
+    if (!force_callback)
+        for (i = 0; choices[i] != AV_PIX_FMT_NONE; i++)
+            if (choices[i] == s->avctx->pix_fmt)
+                return choices[i];
+
     return ff_thread_get_format(s->avctx, pix_fmts);
 }
 
@@ -439,8 +445,6 @@  static int set_sps(HEVCContext *s, const HEVCSPS *sps,
 
     export_stream_params(s->avctx, &s->ps, sps);
 
-    s->avctx->pix_fmt = pix_fmt;
-
     ff_hevc_pred_init(&s->hpc,     sps->bit_depth);
     ff_hevc_dsp_init (&s->hevcdsp, sps->bit_depth);
     ff_videodsp_init (&s->vdsp,    sps->bit_depth);
@@ -526,10 +530,11 @@  static int hls_slice_header(HEVCContext *s)
         if (ret < 0)
             return ret;
 
-        pix_fmt = get_format(s, sps);
+        pix_fmt = get_format(s, sps, s->avctx->hwaccel ? 0 : 1);
         if (pix_fmt < 0)
             return pix_fmt;
-        s->avctx->pix_fmt = pix_fmt;
+        if (s->avctx->pix_fmt != pix_fmt)
+            s->avctx->pix_fmt = pix_fmt;
 
         s->seq_decode = (s->seq_decode + 1) & 0xff;
         s->max_ra     = INT_MAX;