[FFmpeg-devel] examples/hw_decode: Use hw-config information to find pixfmt

This removes all remaining device-type specificity.
---
 doc/examples/hw_decode.c | 55 ++++++++++++++++++++----------------------------
 1 file changed, 23 insertions(+), 32 deletions(-)

On 2017/11/29 8:34, Mark Thompson wrote:
> This removes all remaining device-type specificity.
> ---
>  doc/examples/hw_decode.c | 55 ++++++++++++++++++++----------------------------
>  1 file changed, 23 insertions(+), 32 deletions(-)
>
> diff --git a/doc/examples/hw_decode.c b/doc/examples/hw_decode.c
> index 9c7adbf51a..8f48f3b706 100644
> --- a/doc/examples/hw_decode.c
> +++ b/doc/examples/hw_decode.c
> @@ -44,34 +44,6 @@ static AVBufferRef *hw_device_ctx = NULL;
>  static enum AVPixelFormat hw_pix_fmt;
>  static FILE *output_file = NULL;
>  
> -static enum AVPixelFormat find_fmt_by_hw_type(const enum AVHWDeviceType type)
> -{
> -    enum AVPixelFormat fmt;
> -
> -    switch (type) {
> -    case AV_HWDEVICE_TYPE_VAAPI:
> -        fmt = AV_PIX_FMT_VAAPI;
> -        break;
> -    case AV_HWDEVICE_TYPE_DXVA2:
> -        fmt = AV_PIX_FMT_DXVA2_VLD;
> -        break;
> -    case AV_HWDEVICE_TYPE_D3D11VA:
> -        fmt = AV_PIX_FMT_D3D11;
> -        break;
> -    case AV_HWDEVICE_TYPE_VDPAU:
> -        fmt = AV_PIX_FMT_VDPAU;
> -        break;
> -    case AV_HWDEVICE_TYPE_VIDEOTOOLBOX:
> -        fmt = AV_PIX_FMT_VIDEOTOOLBOX;
> -        break;
> -    default:
> -        fmt = AV_PIX_FMT_NONE;
> -        break;
> -    }
> -
> -    return fmt;
> -}
> -
>  static int hw_decoder_init(AVCodecContext *ctx, const enum AVHWDeviceType type)
>  {
>      int err = 0;
> @@ -184,18 +156,23 @@ int main(int argc, char *argv[])
>      AVCodec *decoder = NULL;
>      AVPacket packet;
>      enum AVHWDeviceType type;
> +    int i;
>  
>      if (argc < 4) {
> -        fprintf(stderr, "Usage: %s <vaapi|vdpau|dxva2|d3d11va> <input file> <output file>\n", argv[0]);
> +        fprintf(stderr, "Usage: %s <device type> <input file> <output file>\n", argv[0]);
>          return -1;
>      }
>  
>      av_register_all();
>  
>      type = av_hwdevice_find_type_by_name(argv[1]);
> -    hw_pix_fmt = find_fmt_by_hw_type(type);
> -    if (hw_pix_fmt == -1) {
> -        fprintf(stderr, "Cannot support '%s' in this example.\n", argv[1]);
> +    if (type == AV_HWDEVICE_TYPE_NONE) {
> +        fprintf(stderr, "Device type %s is not supported.\n", argv[1]);
> +        fprintf(stderr, "Available device types:");
> +        type = AV_HWDEVICE_TYPE_NONE;
> +        while((type = av_hwdevice_iterate_types(type)) != AV_HWDEVICE_TYPE_NONE)
> +            fprintf(stderr, " %s", av_hwdevice_get_type_name(type));
> +        fprintf(stderr, "\n");
>          return -1;
>      }
>  
> @@ -218,6 +195,20 @@ int main(int argc, char *argv[])
>      }
>      video_stream = ret;
>  
> +    for (i = 0;; i++) {
> +        const AVCodecHWConfig *config = avcodec_get_hw_config(decoder, i);
> +        if (!config) {
> +            fprintf(stderr, "Decoder %s does not support device type %s.\n",
> +                    decoder->name, av_hwdevice_get_type_name(type));
> +            return -1;
> +        }
> +        if (config->methods & AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX &&
> +            config->device_type == type) {
> +            hw_pix_fmt = config->pix_fmt;
> +            break;
> +        }
> +    }
> +
>      if (!(decoder_ctx = avcodec_alloc_context3(decoder)))
>          return AVERROR(ENOMEM);
>  
LGTM and tesed

On Wed, Nov 29, 2017 at 00:34:03 +0000, Mark Thompson wrote:
> +    if (type == AV_HWDEVICE_TYPE_NONE) {
> +        fprintf(stderr, "Device type %s is not supported.\n", argv[1]);
> +        fprintf(stderr, "Available device types:");
> +        type = AV_HWDEVICE_TYPE_NONE;
> +        while((type = av_hwdevice_iterate_types(type)) != AV_HWDEVICE_TYPE_NONE)
> +            fprintf(stderr, " %s", av_hwdevice_get_type_name(type));

I see what you're trying to do with "type = AV_HWDEVICE_TYPE_NONE"
(initialize the iterator), but the assignment is redundant.

I guess if the compiler optimizes it away anyway, it may be good for
readability, especially since it's an example. *shrug*

Moritz

Am 29.11.2017 um 13:22 schrieb Moritz Barsnick:
> On Wed, Nov 29, 2017 at 00:34:03 +0000, Mark Thompson wrote:
>> +    if (type == AV_HWDEVICE_TYPE_NONE) {
>> +        fprintf(stderr, "Device type %s is not supported.\n", argv[1]);
>> +        fprintf(stderr, "Available device types:");
>> +        type = AV_HWDEVICE_TYPE_NONE;
>> +        while((type = av_hwdevice_iterate_types(type)) != AV_HWDEVICE_TYPE_NONE)
>> +            fprintf(stderr, " %s", av_hwdevice_get_type_name(type));
> 
> I see what you're trying to do with "type = AV_HWDEVICE_TYPE_NONE"
> (initialize the iterator), but the assignment is redundant.
> 
> I guess if the compiler optimizes it away anyway, it may be good for
> readability, especially since it's an example. *shrug*

I fail to see how the assignment is redundant?
Without the assignment, that would be an infinite loop.

2017-11-29 13:41 GMT+01:00 Timo Rothenpieler <timo@rothenpieler.org>:
> Am 29.11.2017 um 13:22 schrieb Moritz Barsnick:
>>
>> On Wed, Nov 29, 2017 at 00:34:03 +0000, Mark Thompson wrote:
>>>
>>> +    if (type == AV_HWDEVICE_TYPE_NONE) {
>>> +        fprintf(stderr, "Device type %s is not supported.\n", argv[1]);
>>> +        fprintf(stderr, "Available device types:");
>>> +        type = AV_HWDEVICE_TYPE_NONE;
>>> +        while((type = av_hwdevice_iterate_types(type)) !=
>>> AV_HWDEVICE_TYPE_NONE)
>>> +            fprintf(stderr, " %s", av_hwdevice_get_type_name(type));
>>
>>
>> I see what you're trying to do with "type = AV_HWDEVICE_TYPE_NONE"
>> (initialize the iterator), but the assignment is redundant.
>>
>> I guess if the compiler optimizes it away anyway, it may be good for
>> readability, especially since it's an example. *shrug*
>
>
> I fail to see how the assignment is redundant?
> Without the assignment, that would be an infinite loop.

One assignment above...

Carl Eugen

On Wed, Nov 29, 2017 at 13:41:29 +0100, Timo Rothenpieler wrote:
> Am 29.11.2017 um 13:22 schrieb Moritz Barsnick:
> > On Wed, Nov 29, 2017 at 00:34:03 +0000, Mark Thompson wrote:
> >> +    if (type == AV_HWDEVICE_TYPE_NONE) {
> >> +        fprintf(stderr, "Device type %s is not supported.\n", argv[1]);
> >> +        fprintf(stderr, "Available device types:");
> >> +        type = AV_HWDEVICE_TYPE_NONE;
> >> +        while((type = av_hwdevice_iterate_types(type)) != AV_HWDEVICE_TYPE_NONE)
> >> +            fprintf(stderr, " %s", av_hwdevice_get_type_name(type));
> > 
> > I see what you're trying to do with "type = AV_HWDEVICE_TYPE_NONE"
> > (initialize the iterator), but the assignment is redundant.
> [...]
> I fail to see how the assignment is redundant?
> Without the assignment, that would be an infinite loop.

if (type == AV_HWDEVICE_TYPE_NONE) {
    // do something not affecting any variable
    type = AV_HWDEVICE_TYPE_NONE;

Unless something else (what?) modifies the variable "type" in the
meantime, its value was already guaranteed to be AV_HWDEVICE_TYPE_NONE.

What am I missing?

(I didn't mean "(type = av_hwdevice_iterate_types(type)) !=
AV_HWDEVICE_TYPE_NONE", that's why I explicitly quoted the assignment I
meant.)

Moritz

On 29/11/17 12:22, Moritz Barsnick wrote:
> On Wed, Nov 29, 2017 at 00:34:03 +0000, Mark Thompson wrote:
>> +    if (type == AV_HWDEVICE_TYPE_NONE) {
>> +        fprintf(stderr, "Device type %s is not supported.\n", argv[1]);
>> +        fprintf(stderr, "Available device types:");
>> +        type = AV_HWDEVICE_TYPE_NONE;
>> +        while((type = av_hwdevice_iterate_types(type)) != AV_HWDEVICE_TYPE_NONE)
>> +            fprintf(stderr, " %s", av_hwdevice_get_type_name(type));
> 
> I see what you're trying to do with "type = AV_HWDEVICE_TYPE_NONE"
> (initialize the iterator), but the assignment is redundant.
> 
> I guess if the compiler optimizes it away anyway, it may be good for
> readability, especially since it's an example. *shrug*

Huh, I didn't notice that.

I've removed it - the iterating over devices is just to make the error message nicer and mostly irrelevant for the example, so I think avoiding the redundancy wins.

Thanks,

- Mark

On 29/11/17 00:48, Jun Zhao wrote:
> On 2017/11/29 8:34, Mark Thompson wrote:
>> This removes all remaining device-type specificity.
>> ---
>>  doc/examples/hw_decode.c | 55 ++++++++++++++++++++----------------------------
>>  1 file changed, 23 insertions(+), 32 deletions(-)
>>
>> diff --git a/doc/examples/hw_decode.c b/doc/examples/hw_decode.c
>> index 9c7adbf51a..8f48f3b706 100644
>> --- a/doc/examples/hw_decode.c
>> +++ b/doc/examples/hw_decode.c
>> @@ -44,34 +44,6 @@ static AVBufferRef *hw_device_ctx = NULL;
>>  static enum AVPixelFormat hw_pix_fmt;
>>  static FILE *output_file = NULL;
>>  
>> -static enum AVPixelFormat find_fmt_by_hw_type(const enum AVHWDeviceType type)
>> -{
>> -    enum AVPixelFormat fmt;
>> -
>> -    switch (type) {
>> -    case AV_HWDEVICE_TYPE_VAAPI:
>> -        fmt = AV_PIX_FMT_VAAPI;
>> -        break;
>> -    case AV_HWDEVICE_TYPE_DXVA2:
>> -        fmt = AV_PIX_FMT_DXVA2_VLD;
>> -        break;
>> -    case AV_HWDEVICE_TYPE_D3D11VA:
>> -        fmt = AV_PIX_FMT_D3D11;
>> -        break;
>> -    case AV_HWDEVICE_TYPE_VDPAU:
>> -        fmt = AV_PIX_FMT_VDPAU;
>> -        break;
>> -    case AV_HWDEVICE_TYPE_VIDEOTOOLBOX:
>> -        fmt = AV_PIX_FMT_VIDEOTOOLBOX;
>> -        break;
>> -    default:
>> -        fmt = AV_PIX_FMT_NONE;
>> -        break;
>> -    }
>> -
>> -    return fmt;
>> -}
>> -
>>  static int hw_decoder_init(AVCodecContext *ctx, const enum AVHWDeviceType type)
>>  {
>>      int err = 0;
>> @@ -184,18 +156,23 @@ int main(int argc, char *argv[])
>>      AVCodec *decoder = NULL;
>>      AVPacket packet;
>>      enum AVHWDeviceType type;
>> +    int i;
>>  
>>      if (argc < 4) {
>> -        fprintf(stderr, "Usage: %s <vaapi|vdpau|dxva2|d3d11va> <input file> <output file>\n", argv[0]);
>> +        fprintf(stderr, "Usage: %s <device type> <input file> <output file>\n", argv[0]);
>>          return -1;
>>      }
>>  
>>      av_register_all();
>>  
>>      type = av_hwdevice_find_type_by_name(argv[1]);
>> -    hw_pix_fmt = find_fmt_by_hw_type(type);
>> -    if (hw_pix_fmt == -1) {
>> -        fprintf(stderr, "Cannot support '%s' in this example.\n", argv[1]);
>> +    if (type == AV_HWDEVICE_TYPE_NONE) {
>> +        fprintf(stderr, "Device type %s is not supported.\n", argv[1]);
>> +        fprintf(stderr, "Available device types:");
>> +        type = AV_HWDEVICE_TYPE_NONE;
>> +        while((type = av_hwdevice_iterate_types(type)) != AV_HWDEVICE_TYPE_NONE)
>> +            fprintf(stderr, " %s", av_hwdevice_get_type_name(type));
>> +        fprintf(stderr, "\n");
>>          return -1;
>>      }
>>  
>> @@ -218,6 +195,20 @@ int main(int argc, char *argv[])
>>      }
>>      video_stream = ret;
>>  
>> +    for (i = 0;; i++) {
>> +        const AVCodecHWConfig *config = avcodec_get_hw_config(decoder, i);
>> +        if (!config) {
>> +            fprintf(stderr, "Decoder %s does not support device type %s.\n",
>> +                    decoder->name, av_hwdevice_get_type_name(type));
>> +            return -1;
>> +        }
>> +        if (config->methods & AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX &&
>> +            config->device_type == type) {
>> +            hw_pix_fmt = config->pix_fmt;
>> +            break;
>> +        }
>> +    }
>> +
>>      if (!(decoder_ctx = avcodec_alloc_context3(decoder)))
>>          return AVERROR(ENOMEM);
>>  
> LGTM and tesed

Thanks!  Applied with the fixup from Moritz.

- Mark