[FFmpeg-devel,5/6] ffmpeg: Always make the qsv device if the option is set

Previously it was only created if the decode hwaccel was requested;
this makes it unconditionally so we can use it without the hwaccel.
---
 ffmpeg.h     |  4 +---
 ffmpeg_opt.c | 13 ++++++++++++-
 ffmpeg_qsv.c | 14 ++++++--------
 3 files changed, 19 insertions(+), 12 deletions(-)

On Tue, 17 Jan 2017 22:31:48 +0000
Mark Thompson <sw@jkqxz.net> wrote:

> Previously it was only created if the decode hwaccel was requested;
> this makes it unconditionally so we can use it without the hwaccel.
> ---
>  ffmpeg.h     |  4 +---
>  ffmpeg_opt.c | 13 ++++++++++++-
>  ffmpeg_qsv.c | 14 ++++++--------
>  3 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/ffmpeg.h b/ffmpeg.h
> index 75bf50e..7362c2d 100644
> --- a/ffmpeg.h
> +++ b/ffmpeg.h
> @@ -604,9 +604,6 @@ extern const OptionDef options[];
>  extern const HWAccel hwaccels[];
>  extern int hwaccel_lax_profile_check;
>  extern AVBufferRef *hw_device_ctx;
> -#if CONFIG_QSV
> -extern char *qsv_device;
> -#endif
>  
>  
>  void term_init(void);
> @@ -641,6 +638,7 @@ int vdpau_init(AVCodecContext *s);
>  int dxva2_init(AVCodecContext *s);
>  int vda_init(AVCodecContext *s);
>  int videotoolbox_init(AVCodecContext *s);
> +int qsv_device_init(const char *device);
>  int qsv_init(AVCodecContext *s);
>  int qsv_transcode_init(OutputStream *ost);
>  int vaapi_decode_init(AVCodecContext *avctx);
> diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
> index a1c02fc..49397be 100644
> --- a/ffmpeg_opt.c
> +++ b/ffmpeg_opt.c
> @@ -462,6 +462,17 @@ static int opt_vaapi_device(void *optctx, const char *opt, const char *arg)
>  }
>  #endif
>  
> +#if CONFIG_QSV
> +static int opt_qsv_device(void *optctx, const char *opt, const char *arg)
> +{
> +    int err;
> +    err = qsv_device_init(arg);
> +    if (err < 0)
> +        exit_program(1);
> +    return 0;
> +}
> +#endif
> +
>  /**
>   * Parse a metadata specifier passed as 'arg' parameter.
>   * @param arg  metadata string to parse
> @@ -3694,7 +3705,7 @@ const OptionDef options[] = {
>  #endif
>  
>  #if CONFIG_QSV
> -    { "qsv_device", HAS_ARG | OPT_STRING | OPT_EXPERT, { &qsv_device },
> +    { "qsv_device", HAS_ARG | OPT_EXPERT, { .func_arg = opt_qsv_device },
>          "set QSV hardware device (DirectX adapter index, DRM path or X11 display name)", "device"},
>  #endif
>  
> diff --git a/ffmpeg_qsv.c b/ffmpeg_qsv.c
> index 86824b6..9356f05 100644
> --- a/ffmpeg_qsv.c
> +++ b/ffmpeg_qsv.c
> @@ -28,8 +28,6 @@
>  
>  #include "ffmpeg.h"
>  
> -char *qsv_device = NULL;
> -
>  static int qsv_get_buffer(AVCodecContext *s, AVFrame *frame, int flags)
>  {
>      InputStream *ist = s->opaque;
> @@ -43,19 +41,19 @@ static void qsv_uninit(AVCodecContext *s)
>      av_buffer_unref(&ist->hw_frames_ctx);
>  }
>  
> -static int qsv_device_init(InputStream *ist)
> +int qsv_device_init(const char *device)
>  {
>      int err;
>      AVDictionary *dict = NULL;
>  
> -    if (qsv_device) {
> -        err = av_dict_set(&dict, "child_device", qsv_device, 0);
> +    if (device) {
> +        err = av_dict_set(&dict, "child_device", device, 0);
>          if (err < 0)
>              return err;
>      }
>  
>      err = av_hwdevice_ctx_create(&hw_device_ctx, AV_HWDEVICE_TYPE_QSV,
> -                                 ist->hwaccel_device, dict, 0);
> +                                 device, dict, 0);
>      if (err < 0) {
>          av_log(NULL, AV_LOG_ERROR, "Error creating a QSV device\n");
>          goto err_out;
> @@ -76,7 +74,7 @@ int qsv_init(AVCodecContext *s)
>      int ret;
>  
>      if (!hw_device_ctx) {
> -        ret = qsv_device_init(ist);
> +        ret = qsv_device_init(ist->hwaccel_device);
>          if (ret < 0)
>              return ret;
>      }
> @@ -148,7 +146,7 @@ int qsv_transcode_init(OutputStream *ost)
>      av_log(NULL, AV_LOG_VERBOSE, "Setting up QSV transcoding\n");
>  
>      if (!hw_device_ctx) {
> -        err = qsv_device_init(ist);
> +        err = qsv_device_init(ist->hwaccel_device);
>          if (err < 0)
>              goto fail;
>      }

Like the vaapi case, this leaks a device context each time the option
is used after the first time.

Also, shouldn't this maybe only set the hwcontext type, and ffmpeg.c
should call av_hwdevice_ctx_create() in a common code path later?

On 18/01/17 08:01, wm4 wrote:
> On Tue, 17 Jan 2017 22:31:48 +0000
> Mark Thompson <sw@jkqxz.net> wrote:
> 
>> Previously it was only created if the decode hwaccel was requested;
>> this makes it unconditionally so we can use it without the hwaccel.
>> ---
>>  ffmpeg.h     |  4 +---
>>  ffmpeg_opt.c | 13 ++++++++++++-
>>  ffmpeg_qsv.c | 14 ++++++--------
>>  3 files changed, 19 insertions(+), 12 deletions(-)
>>
>> diff --git a/ffmpeg.h b/ffmpeg.h
>> index 75bf50e..7362c2d 100644
>> --- a/ffmpeg.h
>> +++ b/ffmpeg.h
>> @@ -604,9 +604,6 @@ extern const OptionDef options[];
>>  extern const HWAccel hwaccels[];
>>  extern int hwaccel_lax_profile_check;
>>  extern AVBufferRef *hw_device_ctx;
>> -#if CONFIG_QSV
>> -extern char *qsv_device;
>> -#endif
>>  
>>  
>>  void term_init(void);
>> @@ -641,6 +638,7 @@ int vdpau_init(AVCodecContext *s);
>>  int dxva2_init(AVCodecContext *s);
>>  int vda_init(AVCodecContext *s);
>>  int videotoolbox_init(AVCodecContext *s);
>> +int qsv_device_init(const char *device);
>>  int qsv_init(AVCodecContext *s);
>>  int qsv_transcode_init(OutputStream *ost);
>>  int vaapi_decode_init(AVCodecContext *avctx);
>> diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
>> index a1c02fc..49397be 100644
>> --- a/ffmpeg_opt.c
>> +++ b/ffmpeg_opt.c
>> @@ -462,6 +462,17 @@ static int opt_vaapi_device(void *optctx, const char *opt, const char *arg)
>>  }
>>  #endif
>>  
>> +#if CONFIG_QSV
>> +static int opt_qsv_device(void *optctx, const char *opt, const char *arg)
>> +{
>> +    int err;
>> +    err = qsv_device_init(arg);
>> +    if (err < 0)
>> +        exit_program(1);
>> +    return 0;
>> +}
>> +#endif
>> +
>>  /**
>>   * Parse a metadata specifier passed as 'arg' parameter.
>>   * @param arg  metadata string to parse
>> @@ -3694,7 +3705,7 @@ const OptionDef options[] = {
>>  #endif
>>  
>>  #if CONFIG_QSV
>> -    { "qsv_device", HAS_ARG | OPT_STRING | OPT_EXPERT, { &qsv_device },
>> +    { "qsv_device", HAS_ARG | OPT_EXPERT, { .func_arg = opt_qsv_device },
>>          "set QSV hardware device (DirectX adapter index, DRM path or X11 display name)", "device"},
>>  #endif
>>  
>> diff --git a/ffmpeg_qsv.c b/ffmpeg_qsv.c
>> index 86824b6..9356f05 100644
>> --- a/ffmpeg_qsv.c
>> +++ b/ffmpeg_qsv.c
>> @@ -28,8 +28,6 @@
>>  
>>  #include "ffmpeg.h"
>>  
>> -char *qsv_device = NULL;
>> -
>>  static int qsv_get_buffer(AVCodecContext *s, AVFrame *frame, int flags)
>>  {
>>      InputStream *ist = s->opaque;
>> @@ -43,19 +41,19 @@ static void qsv_uninit(AVCodecContext *s)
>>      av_buffer_unref(&ist->hw_frames_ctx);
>>  }
>>  
>> -static int qsv_device_init(InputStream *ist)
>> +int qsv_device_init(const char *device)
>>  {
>>      int err;
>>      AVDictionary *dict = NULL;
>>  
>> -    if (qsv_device) {
>> -        err = av_dict_set(&dict, "child_device", qsv_device, 0);
>> +    if (device) {
>> +        err = av_dict_set(&dict, "child_device", device, 0);
>>          if (err < 0)
>>              return err;
>>      }
>>  
>>      err = av_hwdevice_ctx_create(&hw_device_ctx, AV_HWDEVICE_TYPE_QSV,
>> -                                 ist->hwaccel_device, dict, 0);
>> +                                 device, dict, 0);
>>      if (err < 0) {
>>          av_log(NULL, AV_LOG_ERROR, "Error creating a QSV device\n");
>>          goto err_out;
>> @@ -76,7 +74,7 @@ int qsv_init(AVCodecContext *s)
>>      int ret;
>>  
>>      if (!hw_device_ctx) {
>> -        ret = qsv_device_init(ist);
>> +        ret = qsv_device_init(ist->hwaccel_device);
>>          if (ret < 0)
>>              return ret;
>>      }
>> @@ -148,7 +146,7 @@ int qsv_transcode_init(OutputStream *ost)
>>      av_log(NULL, AV_LOG_VERBOSE, "Setting up QSV transcoding\n");
>>  
>>      if (!hw_device_ctx) {
>> -        err = qsv_device_init(ist);
>> +        err = qsv_device_init(ist->hwaccel_device);
>>          if (err < 0)
>>              goto fail;
>>      }
> 
> Like the vaapi case, this leaks a device context each time the option
> is used after the first time.

Right, I can fix that.  (For VAAPI too.)

> Also, shouldn't this maybe only set the hwcontext type, and ffmpeg.c
> should call av_hwdevice_ctx_create() in a common code path later?

Is there any case where the ordering matters?

(It does have to be created unconditionally once you have the option, because it needs to be set for filters to work.  The skipped scale_qsv and subsequent deinterlace_qsv filters can be merged to work with hwupload after this.)

On Sat, 21 Jan 2017 22:16:15 +0000
Mark Thompson <sw@jkqxz.net> wrote:

> On 18/01/17 08:01, wm4 wrote:
> > On Tue, 17 Jan 2017 22:31:48 +0000
> > Mark Thompson <sw@jkqxz.net> wrote:
> >   
> >> Previously it was only created if the decode hwaccel was requested;
> >> this makes it unconditionally so we can use it without the hwaccel.
> >> ---
> >>  ffmpeg.h     |  4 +---
> >>  ffmpeg_opt.c | 13 ++++++++++++-
> >>  ffmpeg_qsv.c | 14 ++++++--------
> >>  3 files changed, 19 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/ffmpeg.h b/ffmpeg.h
> >> index 75bf50e..7362c2d 100644
> >> --- a/ffmpeg.h
> >> +++ b/ffmpeg.h
> >> @@ -604,9 +604,6 @@ extern const OptionDef options[];
> >>  extern const HWAccel hwaccels[];
> >>  extern int hwaccel_lax_profile_check;
> >>  extern AVBufferRef *hw_device_ctx;
> >> -#if CONFIG_QSV
> >> -extern char *qsv_device;
> >> -#endif
> >>  
> >>  
> >>  void term_init(void);
> >> @@ -641,6 +638,7 @@ int vdpau_init(AVCodecContext *s);
> >>  int dxva2_init(AVCodecContext *s);
> >>  int vda_init(AVCodecContext *s);
> >>  int videotoolbox_init(AVCodecContext *s);
> >> +int qsv_device_init(const char *device);
> >>  int qsv_init(AVCodecContext *s);
> >>  int qsv_transcode_init(OutputStream *ost);
> >>  int vaapi_decode_init(AVCodecContext *avctx);
> >> diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
> >> index a1c02fc..49397be 100644
> >> --- a/ffmpeg_opt.c
> >> +++ b/ffmpeg_opt.c
> >> @@ -462,6 +462,17 @@ static int opt_vaapi_device(void *optctx, const char *opt, const char *arg)
> >>  }
> >>  #endif
> >>  
> >> +#if CONFIG_QSV
> >> +static int opt_qsv_device(void *optctx, const char *opt, const char *arg)
> >> +{
> >> +    int err;
> >> +    err = qsv_device_init(arg);
> >> +    if (err < 0)
> >> +        exit_program(1);
> >> +    return 0;
> >> +}
> >> +#endif
> >> +
> >>  /**
> >>   * Parse a metadata specifier passed as 'arg' parameter.
> >>   * @param arg  metadata string to parse
> >> @@ -3694,7 +3705,7 @@ const OptionDef options[] = {
> >>  #endif
> >>  
> >>  #if CONFIG_QSV
> >> -    { "qsv_device", HAS_ARG | OPT_STRING | OPT_EXPERT, { &qsv_device },
> >> +    { "qsv_device", HAS_ARG | OPT_EXPERT, { .func_arg = opt_qsv_device },
> >>          "set QSV hardware device (DirectX adapter index, DRM path or X11 display name)", "device"},
> >>  #endif
> >>  
> >> diff --git a/ffmpeg_qsv.c b/ffmpeg_qsv.c
> >> index 86824b6..9356f05 100644
> >> --- a/ffmpeg_qsv.c
> >> +++ b/ffmpeg_qsv.c
> >> @@ -28,8 +28,6 @@
> >>  
> >>  #include "ffmpeg.h"
> >>  
> >> -char *qsv_device = NULL;
> >> -
> >>  static int qsv_get_buffer(AVCodecContext *s, AVFrame *frame, int flags)
> >>  {
> >>      InputStream *ist = s->opaque;
> >> @@ -43,19 +41,19 @@ static void qsv_uninit(AVCodecContext *s)
> >>      av_buffer_unref(&ist->hw_frames_ctx);
> >>  }
> >>  
> >> -static int qsv_device_init(InputStream *ist)
> >> +int qsv_device_init(const char *device)
> >>  {
> >>      int err;
> >>      AVDictionary *dict = NULL;
> >>  
> >> -    if (qsv_device) {
> >> -        err = av_dict_set(&dict, "child_device", qsv_device, 0);
> >> +    if (device) {
> >> +        err = av_dict_set(&dict, "child_device", device, 0);
> >>          if (err < 0)
> >>              return err;
> >>      }
> >>  
> >>      err = av_hwdevice_ctx_create(&hw_device_ctx, AV_HWDEVICE_TYPE_QSV,
> >> -                                 ist->hwaccel_device, dict, 0);
> >> +                                 device, dict, 0);
> >>      if (err < 0) {
> >>          av_log(NULL, AV_LOG_ERROR, "Error creating a QSV device\n");
> >>          goto err_out;
> >> @@ -76,7 +74,7 @@ int qsv_init(AVCodecContext *s)
> >>      int ret;
> >>  
> >>      if (!hw_device_ctx) {
> >> -        ret = qsv_device_init(ist);
> >> +        ret = qsv_device_init(ist->hwaccel_device);
> >>          if (ret < 0)
> >>              return ret;
> >>      }
> >> @@ -148,7 +146,7 @@ int qsv_transcode_init(OutputStream *ost)
> >>      av_log(NULL, AV_LOG_VERBOSE, "Setting up QSV transcoding\n");
> >>  
> >>      if (!hw_device_ctx) {
> >> -        err = qsv_device_init(ist);
> >> +        err = qsv_device_init(ist->hwaccel_device);
> >>          if (err < 0)
> >>              goto fail;
> >>      }  
> > 
> > Like the vaapi case, this leaks a device context each time the option
> > is used after the first time.  
> 
> Right, I can fix that.  (For VAAPI too.)
> 
> > Also, shouldn't this maybe only set the hwcontext type, and ffmpeg.c
> > should call av_hwdevice_ctx_create() in a common code path later?  
> 
> Is there any case where the ordering matters?
> 
> (It does have to be created unconditionally once you have the option, because it needs to be set for filters to work.  The skipped scale_qsv and subsequent deinterlace_qsv filters can be merged to work with hwupload after this.)

Probably would be a separate change at a much later time anyway.