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

Submitted by Mark Thompson on Nov. 29, 2017, 12:34 a.m.

Details

Message ID 39281f79-0625-8428-66b0-ef97acfc2c2f@jkqxz.net
State New
Headers show

Commit Message

Mark Thompson Nov. 29, 2017, 12:34 a.m.
This removes all remaining device-type specificity.
---
 doc/examples/hw_decode.c | 55 ++++++++++++++++++++----------------------------
 1 file changed, 23 insertions(+), 32 deletions(-)

Comments

Jun Zhao Nov. 29, 2017, 12:48 a.m.
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
Moritz Barsnick Nov. 29, 2017, 12:22 p.m.
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
Timo Rothenpieler Nov. 29, 2017, 12:41 p.m.
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.
Carl Eugen Hoyos Nov. 29, 2017, 1 p.m.
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
Moritz Barsnick Nov. 29, 2017, 2:17 p.m.
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
Mark Thompson Nov. 29, 2017, 11:48 p.m.
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
Mark Thompson Nov. 29, 2017, 11:59 p.m.
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

Patch hide | download patch | download mbox

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);