[FFmpeg-devel,v2,1/3] avutil/hwcontext: add a function to get the AVHWDeviceType

Add a function to get the corresponding AVHWDeviceType from a given
hardware pixel format.

Signed-off-by: Tong Wu <tong1.wu@intel.com>
---
 libavutil/hwcontext.c | 12 ++++++++++++
 libavutil/hwcontext.h |  9 +++++++++
 2 files changed, 21 insertions(+)

Quoting Tong Wu (2022-06-30 04:45:56)
> Add a function to get the corresponding AVHWDeviceType from a given
> hardware pixel format.
> 
> Signed-off-by: Tong Wu <tong1.wu@intel.com>
> ---
>  libavutil/hwcontext.c | 12 ++++++++++++
>  libavutil/hwcontext.h |  9 +++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
> index ab9ad3703e..3521ed34f4 100644
> --- a/libavutil/hwcontext.c
> +++ b/libavutil/hwcontext.c
> @@ -80,6 +80,18 @@ static const char *const hw_type_names[] = {
>      [AV_HWDEVICE_TYPE_VULKAN] = "vulkan",
>  };
>  
> +enum AVHWDeviceType av_hwdevice_get_type_by_pix_fmt(enum AVPixelFormat fmt)
> +{
> +    int i, j;

Nit: you can and should declare loop indices in the loop statement
itself

> +    for (i = 0; hw_table[i]; i++) {
> +        for (j = 0; hw_table[i]->pix_fmts[j] != AV_PIX_FMT_NONE; j++) {
> +            if (hw_table[i]->pix_fmts[j] == fmt)
> +                return hw_table[i]->type;
> +        }
> +    }
> +    return AV_HWDEVICE_TYPE_NONE;
> +}
> +
>  enum AVHWDeviceType av_hwdevice_find_type_by_name(const char *name)
>  {
>      int type;
> diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
> index c18b7e1e8b..97f94403e2 100644
> --- a/libavutil/hwcontext.h
> +++ b/libavutil/hwcontext.h
> @@ -229,6 +229,15 @@ typedef struct AVHWFramesContext {
>      int width, height;
>  } AVHWFramesContext;
>  
> +/**
> + * Get the device type by a given pixel format.
> + *
> + * @param fmt Pixel format from enum AVPixelFormat.
> + * @return The type from enum AVHWDeviceType, or AV_HWDEVICE_TYPE_NONE if
> + *         not found.
> + */
> +enum AVHWDeviceType av_hwdevice_get_type_by_pix_fmt(enum AVPixelFormat fmt);

I wonder if we should consider the possibility of a format being
supported by more than one device type.

> On Jun 30, 2022, at 4:43 PM, Anton Khirnov <anton@khirnov.net> wrote:
> 
> Quoting Tong Wu (2022-06-30 04:45:56)
>> Add a function to get the corresponding AVHWDeviceType from a given
>> hardware pixel format.
>> 
>> Signed-off-by: Tong Wu <tong1.wu@intel.com>
>> ---
>> libavutil/hwcontext.c | 12 ++++++++++++
>> libavutil/hwcontext.h |  9 +++++++++
>> 2 files changed, 21 insertions(+)
>> 
>> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
>> index ab9ad3703e..3521ed34f4 100644
>> --- a/libavutil/hwcontext.c
>> +++ b/libavutil/hwcontext.c
>> @@ -80,6 +80,18 @@ static const char *const hw_type_names[] = {
>>     [AV_HWDEVICE_TYPE_VULKAN] = "vulkan",
>> };
>> 
>> +enum AVHWDeviceType av_hwdevice_get_type_by_pix_fmt(enum AVPixelFormat fmt)
>> +{
>> +    int i, j;
> 
> Nit: you can and should declare loop indices in the loop statement
> itself
> 
>> +    for (i = 0; hw_table[i]; i++) {
>> +        for (j = 0; hw_table[i]->pix_fmts[j] != AV_PIX_FMT_NONE; j++) {
>> +            if (hw_table[i]->pix_fmts[j] == fmt)
>> +                return hw_table[i]->type;
>> +        }
>> +    }
>> +    return AV_HWDEVICE_TYPE_NONE;
>> +}
>> +
>> enum AVHWDeviceType av_hwdevice_find_type_by_name(const char *name)
>> {
>>     int type;
>> diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
>> index c18b7e1e8b..97f94403e2 100644
>> --- a/libavutil/hwcontext.h
>> +++ b/libavutil/hwcontext.h
>> @@ -229,6 +229,15 @@ typedef struct AVHWFramesContext {
>>     int width, height;
>> } AVHWFramesContext;
>> 
>> +/**
>> + * Get the device type by a given pixel format.
>> + *
>> + * @param fmt Pixel format from enum AVPixelFormat.
>> + * @return The type from enum AVHWDeviceType, or AV_HWDEVICE_TYPE_NONE if
>> + *         not found.
>> + */
>> +enum AVHWDeviceType av_hwdevice_get_type_by_pix_fmt(enum AVPixelFormat fmt);
> 
> I wonder if we should consider the possibility of a format being
> supported by more than one device type.

For future proof, we can make it clear that there is no guarantee
that the device type is unique, e.g.,

"Get any device type which support the given pixel format”

> 
> -- 
> Anton Khirnov
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

"zhilizhao(赵志立)":
> 
> 
>> On Jun 30, 2022, at 4:43 PM, Anton Khirnov <anton@khirnov.net> wrote:
>>
>> Quoting Tong Wu (2022-06-30 04:45:56)
>>> Add a function to get the corresponding AVHWDeviceType from a given
>>> hardware pixel format.
>>>
>>> Signed-off-by: Tong Wu <tong1.wu@intel.com>
>>> ---
>>> libavutil/hwcontext.c | 12 ++++++++++++
>>> libavutil/hwcontext.h |  9 +++++++++
>>> 2 files changed, 21 insertions(+)
>>>
>>> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
>>> index ab9ad3703e..3521ed34f4 100644
>>> --- a/libavutil/hwcontext.c
>>> +++ b/libavutil/hwcontext.c
>>> @@ -80,6 +80,18 @@ static const char *const hw_type_names[] = {
>>>     [AV_HWDEVICE_TYPE_VULKAN] = "vulkan",
>>> };
>>>
>>> +enum AVHWDeviceType av_hwdevice_get_type_by_pix_fmt(enum AVPixelFormat fmt)
>>> +{
>>> +    int i, j;
>>
>> Nit: you can and should declare loop indices in the loop statement
>> itself
>>
>>> +    for (i = 0; hw_table[i]; i++) {
>>> +        for (j = 0; hw_table[i]->pix_fmts[j] != AV_PIX_FMT_NONE; j++) {
>>> +            if (hw_table[i]->pix_fmts[j] == fmt)
>>> +                return hw_table[i]->type;
>>> +        }
>>> +    }
>>> +    return AV_HWDEVICE_TYPE_NONE;
>>> +}
>>> +
>>> enum AVHWDeviceType av_hwdevice_find_type_by_name(const char *name)
>>> {
>>>     int type;
>>> diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
>>> index c18b7e1e8b..97f94403e2 100644
>>> --- a/libavutil/hwcontext.h
>>> +++ b/libavutil/hwcontext.h
>>> @@ -229,6 +229,15 @@ typedef struct AVHWFramesContext {
>>>     int width, height;
>>> } AVHWFramesContext;
>>>
>>> +/**
>>> + * Get the device type by a given pixel format.
>>> + *
>>> + * @param fmt Pixel format from enum AVPixelFormat.
>>> + * @return The type from enum AVHWDeviceType, or AV_HWDEVICE_TYPE_NONE if
>>> + *         not found.
>>> + */
>>> +enum AVHWDeviceType av_hwdevice_get_type_by_pix_fmt(enum AVPixelFormat fmt);
>>
>> I wonder if we should consider the possibility of a format being
>> supported by more than one device type.
> 
> For future proof, we can make it clear that there is no guarantee
> that the device type is unique, e.g.,
> 
> "Get any device type which support the given pixel format”
> 

Then you'd need to return a list or modify the user accept an iterator.

- Andreas

> On Jun 30, 2022, at 7:56 PM, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 
> "zhilizhao(赵志立)":
>> 
>> 
>>> On Jun 30, 2022, at 4:43 PM, Anton Khirnov <anton@khirnov.net> wrote:
>>> 
>>> Quoting Tong Wu (2022-06-30 04:45:56)
>>>> Add a function to get the corresponding AVHWDeviceType from a given
>>>> hardware pixel format.
>>>> 
>>>> Signed-off-by: Tong Wu <tong1.wu@intel.com>
>>>> ---
>>>> libavutil/hwcontext.c | 12 ++++++++++++
>>>> libavutil/hwcontext.h |  9 +++++++++
>>>> 2 files changed, 21 insertions(+)
>>>> 
>>>> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
>>>> index ab9ad3703e..3521ed34f4 100644
>>>> --- a/libavutil/hwcontext.c
>>>> +++ b/libavutil/hwcontext.c
>>>> @@ -80,6 +80,18 @@ static const char *const hw_type_names[] = {
>>>>    [AV_HWDEVICE_TYPE_VULKAN] = "vulkan",
>>>> };
>>>> 
>>>> +enum AVHWDeviceType av_hwdevice_get_type_by_pix_fmt(enum AVPixelFormat fmt)
>>>> +{
>>>> +    int i, j;
>>> 
>>> Nit: you can and should declare loop indices in the loop statement
>>> itself
>>> 
>>>> +    for (i = 0; hw_table[i]; i++) {
>>>> +        for (j = 0; hw_table[i]->pix_fmts[j] != AV_PIX_FMT_NONE; j++) {
>>>> +            if (hw_table[i]->pix_fmts[j] == fmt)
>>>> +                return hw_table[i]->type;
>>>> +        }
>>>> +    }
>>>> +    return AV_HWDEVICE_TYPE_NONE;
>>>> +}
>>>> +
>>>> enum AVHWDeviceType av_hwdevice_find_type_by_name(const char *name)
>>>> {
>>>>    int type;
>>>> diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h
>>>> index c18b7e1e8b..97f94403e2 100644
>>>> --- a/libavutil/hwcontext.h
>>>> +++ b/libavutil/hwcontext.h
>>>> @@ -229,6 +229,15 @@ typedef struct AVHWFramesContext {
>>>>    int width, height;
>>>> } AVHWFramesContext;
>>>> 
>>>> +/**
>>>> + * Get the device type by a given pixel format.
>>>> + *
>>>> + * @param fmt Pixel format from enum AVPixelFormat.
>>>> + * @return The type from enum AVHWDeviceType, or AV_HWDEVICE_TYPE_NONE if
>>>> + *         not found.
>>>> + */
>>>> +enum AVHWDeviceType av_hwdevice_get_type_by_pix_fmt(enum AVPixelFormat fmt);
>>> 
>>> I wonder if we should consider the possibility of a format being
>>> supported by more than one device type.
>> 
>> For future proof, we can make it clear that there is no guarantee
>> that the device type is unique, e.g.,
>> 
>> "Get any device type which support the given pixel format”
>> 
> 
> Then you'd need to return a list or modify the user accept an iterator.

Iterator should be fine.

However, the use case is unclear: since we only return an AVHWDeviceType
without description like av_pix_fmt_desc_get(), user has little information
to skip to the next one, unless user only wants to get all of the device
types which support a pixel format. 

> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

> > On Jun 30, 2022, at 7:56 PM, Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
> >
> > "zhilizhao(赵志立)":
> >>
> >>
> >>> On Jun 30, 2022, at 4:43 PM, Anton Khirnov <anton@khirnov.net> wrote:
> >>>
> >>> Quoting Tong Wu (2022-06-30 04:45:56)
> >>>> Add a function to get the corresponding AVHWDeviceType from a given
> >>>> hardware pixel format.
> >>>>
> >>>> Signed-off-by: Tong Wu <tong1.wu@intel.com>
> >>>> ---
> >>>> libavutil/hwcontext.c | 12 ++++++++++++ libavutil/hwcontext.h |  9
> >>>> +++++++++
> >>>> 2 files changed, 21 insertions(+)
> >>>>
> >>>> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c index
> >>>> ab9ad3703e..3521ed34f4 100644
> >>>> --- a/libavutil/hwcontext.c
> >>>> +++ b/libavutil/hwcontext.c
> >>>> @@ -80,6 +80,18 @@ static const char *const hw_type_names[] = {
> >>>>    [AV_HWDEVICE_TYPE_VULKAN] = "vulkan", };
> >>>>
> >>>> +enum AVHWDeviceType av_hwdevice_get_type_by_pix_fmt(enum
> >>>> +AVPixelFormat fmt) {
> >>>> +    int i, j;
> >>>
> >>> Nit: you can and should declare loop indices in the loop statement
> >>> itself

Sure, will modify them in v3.

> >>>
> >>>> +    for (i = 0; hw_table[i]; i++) {
> >>>> +        for (j = 0; hw_table[i]->pix_fmts[j] != AV_PIX_FMT_NONE; j++) {
> >>>> +            if (hw_table[i]->pix_fmts[j] == fmt)
> >>>> +                return hw_table[i]->type;
> >>>> +        }
> >>>> +    }
> >>>> +    return AV_HWDEVICE_TYPE_NONE;
> >>>> +}
> >>>> +
> >>>> enum AVHWDeviceType av_hwdevice_find_type_by_name(const char
> *name)
> >>>> {
> >>>>    int type;
> >>>> diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h index
> >>>> c18b7e1e8b..97f94403e2 100644
> >>>> --- a/libavutil/hwcontext.h
> >>>> +++ b/libavutil/hwcontext.h
> >>>> @@ -229,6 +229,15 @@ typedef struct AVHWFramesContext {
> >>>>    int width, height;
> >>>> } AVHWFramesContext;
> >>>>
> >>>> +/**
> >>>> + * Get the device type by a given pixel format.
> >>>> + *
> >>>> + * @param fmt Pixel format from enum AVPixelFormat.
> >>>> + * @return The type from enum AVHWDeviceType, or
> AV_HWDEVICE_TYPE_NONE if
> >>>> + *         not found.
> >>>> + */
> >>>> +enum AVHWDeviceType av_hwdevice_get_type_by_pix_fmt(enum
> >>>> +AVPixelFormat fmt);
> >>>
> >>> I wonder if we should consider the possibility of a format being
> >>> supported by more than one device type.
> >>
> >> For future proof, we can make it clear that there is no guarantee
> >> that the device type is unique, e.g.,
> >>
> >> "Get any device type which support the given pixel format”
> >>
> >
> > Then you'd need to return a list or modify the user accept an iterator.
> 
> Iterator should be fine.
> 
> However, the use case is unclear: since we only return an AVHWDeviceType
> without description like av_pix_fmt_desc_get(), user has little information to
> skip to the next one, unless user only wants to get all of the device types
> which support a pixel format.

Since so far there's no hardware format being supported by more than one hardware device, can we just keep current implementation and clarify it in comments such as "Get any device type which support the given pixel format”? 

Thanks,
Tong

> 
> >
> > - Andreas
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
> with subject "unsubscribe".

> > > On Jun 30, 2022, at 7:56 PM, Andreas Rheinhardt
> > <andreas.rheinhardt@outlook.com> wrote:
> > >
> > > "zhilizhao(赵志立)":
> > >>
> > >>
> > >>> On Jun 30, 2022, at 4:43 PM, Anton Khirnov <anton@khirnov.net>
> wrote:
> > >>>
> > >>> Quoting Tong Wu (2022-06-30 04:45:56)
> > >>>> Add a function to get the corresponding AVHWDeviceType from a
> > >>>> given hardware pixel format.
> > >>>>
> > >>>> Signed-off-by: Tong Wu <tong1.wu@intel.com>
> > >>>> ---
> > >>>> libavutil/hwcontext.c | 12 ++++++++++++ libavutil/hwcontext.h |
> > >>>> 9
> > >>>> +++++++++
> > >>>> 2 files changed, 21 insertions(+)
> > >>>>
> > >>>> diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c index
> > >>>> ab9ad3703e..3521ed34f4 100644
> > >>>> --- a/libavutil/hwcontext.c
> > >>>> +++ b/libavutil/hwcontext.c
> > >>>> @@ -80,6 +80,18 @@ static const char *const hw_type_names[] = {
> > >>>>    [AV_HWDEVICE_TYPE_VULKAN] = "vulkan", };
> > >>>>
> > >>>> +enum AVHWDeviceType av_hwdevice_get_type_by_pix_fmt(enum
> > >>>> +AVPixelFormat fmt) {
> > >>>> +    int i, j;
> > >>>
> > >>> Nit: you can and should declare loop indices in the loop statement
> > >>> itself
> 
> Sure, will modify them in v3.
> 
> > >>>
> > >>>> +    for (i = 0; hw_table[i]; i++) {
> > >>>> +        for (j = 0; hw_table[i]->pix_fmts[j] != AV_PIX_FMT_NONE; j++) {
> > >>>> +            if (hw_table[i]->pix_fmts[j] == fmt)
> > >>>> +                return hw_table[i]->type;
> > >>>> +        }
> > >>>> +    }
> > >>>> +    return AV_HWDEVICE_TYPE_NONE; }
> > >>>> +
> > >>>> enum AVHWDeviceType av_hwdevice_find_type_by_name(const char
> > *name)
> > >>>> {
> > >>>>    int type;
> > >>>> diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h index
> > >>>> c18b7e1e8b..97f94403e2 100644
> > >>>> --- a/libavutil/hwcontext.h
> > >>>> +++ b/libavutil/hwcontext.h
> > >>>> @@ -229,6 +229,15 @@ typedef struct AVHWFramesContext {
> > >>>>    int width, height;
> > >>>> } AVHWFramesContext;
> > >>>>
> > >>>> +/**
> > >>>> + * Get the device type by a given pixel format.
> > >>>> + *
> > >>>> + * @param fmt Pixel format from enum AVPixelFormat.
> > >>>> + * @return The type from enum AVHWDeviceType, or
> > AV_HWDEVICE_TYPE_NONE if
> > >>>> + *         not found.
> > >>>> + */
> > >>>> +enum AVHWDeviceType av_hwdevice_get_type_by_pix_fmt(enum
> > >>>> +AVPixelFormat fmt);
> > >>>
> > >>> I wonder if we should consider the possibility of a format being
> > >>> supported by more than one device type.
> > >>
> > >> For future proof, we can make it clear that there is no guarantee
> > >> that the device type is unique, e.g.,
> > >>
> > >> "Get any device type which support the given pixel format”
> > >>
> > >
> > > Then you'd need to return a list or modify the user accept an iterator.
> >
> > Iterator should be fine.
> >
> > However, the use case is unclear: since we only return an
> > AVHWDeviceType without description like av_pix_fmt_desc_get(), user
> > has little information to skip to the next one, unless user only wants
> > to get all of the device types which support a pixel format.
> 
> Since so far there's no hardware format being supported by more than one
> hardware device, can we just keep current implementation and clarify it in
> comments such as "Get any device type which support the given pixel
> format”?
> 
> Thanks,
> Tong
> 

Plus, do you think adding a AV_PIX_FMT_FLAG_HWACCEL check for the input pixel format and change the function name to av_hwdevice_get_type_by_hwaccel_pix_fmt will help? Let users know only hardware pixel formats are acceptable for this function and like I said at this moment not any hardware format is supported by more than one hardware devices.

Tong

> >
> > >
> > > - Andreas
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
> with subject "unsubscribe".

Quoting Wu, Tong1 (2022-07-01 08:51:28)
> Plus, do you think adding a AV_PIX_FMT_FLAG_HWACCEL check for the
> input pixel format and change the function name to
> av_hwdevice_get_type_by_hwaccel_pix_fmt will help?

I'd prefer not to, that name is way too long

> Let users know only hardware pixel formats are acceptable for this
> function and like I said at this moment not any hardware format is
> supported by more than one hardware devices.

Maybe just document in the doxy that it will return the preferred device
type for a pixel format.

Overall I don't insist on any changes here, mainly wondering if anyone
can think of a potential future case where we'd want a single pixel
format supported by multiple devices

> Quoting Wu, Tong1 (2022-07-01 08:51:28)
> > Plus, do you think adding a AV_PIX_FMT_FLAG_HWACCEL check for the
> > input pixel format and change the function name to
> > av_hwdevice_get_type_by_hwaccel_pix_fmt will help?
> 
> I'd prefer not to, that name is way too long
> 
> > Let users know only hardware pixel formats are acceptable for this
> > function and like I said at this moment not any hardware format is
> > supported by more than one hardware devices.
> 
> Maybe just document in the doxy that it will return the preferred device type
> for a pixel format.

Sure, will do it in v3. Thanks for the comments.

> 
> Overall I don't insist on any changes here, mainly wondering if anyone can
> think of a potential future case where we'd want a single pixel format
> supported by multiple devices
> 
> --
> Anton Khirnov
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
> with subject "unsubscribe".