diff mbox series

[FFmpeg-devel,21/35] avdevice: capabilities API details no longer public

Message ID 20210607230414.612-22-dcnieho@gmail.com
State Superseded, archived
Headers show
Series avdevice (mostly dshow) enhancements
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Diederick C. Niehorster June 7, 2021, 11:04 p.m. UTC
NB: will break build, makes needed corresponding changes to avformat.

Signed-off-by: Diederick Niehorster <dcnieho@gmail.com>
---
 libavdevice/avdevice.c | 34 ++++++++++++++++++++--------------
 libavdevice/avdevice.h | 42 +++++++++---------------------------------
 libavdevice/internal.h | 33 +++++++++++++++++++++++++++++++++
 libavdevice/version.h  |  2 +-
 4 files changed, 63 insertions(+), 48 deletions(-)

Comments

James Almer June 7, 2021, 11:26 p.m. UTC | #1
On 6/7/2021 8:04 PM, Diederick Niehorster wrote:
> NB: will break build, makes needed corresponding changes to avformat.
> 
> Signed-off-by: Diederick Niehorster <dcnieho@gmail.com>
> ---
>   libavdevice/avdevice.c | 34 ++++++++++++++++++++--------------
>   libavdevice/avdevice.h | 42 +++++++++---------------------------------
>   libavdevice/internal.h | 33 +++++++++++++++++++++++++++++++++
>   libavdevice/version.h  |  2 +-
>   4 files changed, 63 insertions(+), 48 deletions(-)
> 
> diff --git a/libavdevice/avdevice.c b/libavdevice/avdevice.c
> index d73d36e1dd..b521516ff5 100644
> --- a/libavdevice/avdevice.c
> +++ b/libavdevice/avdevice.c
> @@ -96,51 +96,57 @@ int avdevice_dev_to_app_control_message(struct AVFormatContext *s, enum AVDevToA
>       return s->control_message_cb(s, type, data, data_size);
>   }
>   
> -int avdevice_capabilities_create(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s,
> +int avdevice_capabilities_create(void** opaque, AVFormatContext *s,
>                                    AVDictionary **device_options)
>   {
>       int ret;
> +    AVDeviceCapabilitiesQuery *caps = NULL;
>       av_assert0(s);
> -    av_assert0(caps);
> +    av_assert0(opaque);
>       av_assert0(s->iformat || s->oformat);
> +    *opaque = NULL;
>       if ((s->oformat && !s->oformat->create_device_capabilities) ||
>           (s->iformat && !s->iformat->create_device_capabilities)) {
> -        *caps = NULL;
>           return AVERROR(ENOSYS);
>       }
> -    *caps = av_mallocz(sizeof(AVDeviceCapabilitiesQuery));
> -    if (!(*caps))
> +    *opaque = caps = av_mallocz(sizeof(AVDeviceCapabilitiesQuery));
> +    if (!caps)
>           return AVERROR(ENOMEM);
> -    (*caps)->device_context = s;
> +    caps->device_context = s;
>       if (((ret = av_opt_set_dict(s->priv_data, device_options)) < 0))
>           goto fail;
>       if (s->iformat) {
> -        if ((ret = s->iformat->create_device_capabilities(s, *caps)) < 0)
> +        if ((ret = s->iformat->create_device_capabilities(s, caps)) < 0)
>               goto fail;
>       } else {
> -        if ((ret = s->oformat->create_device_capabilities(s, *caps)) < 0)
> +        if ((ret = s->oformat->create_device_capabilities(s, caps)) < 0)
>               goto fail;
>       }
> -    av_opt_set_defaults(*caps);
> +    av_opt_set_defaults(caps);
>       return 0;
>     fail:
>       av_freep(caps);
> +    *opaque = NULL;
>       return ret;
>   }
>   
> -void avdevice_capabilities_free(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s)
> +void avdevice_capabilities_free(void **opaque, AVFormatContext *s)
>   {
> -    if (!s || !caps || !(*caps))
> +    AVDeviceCapabilitiesQuery *caps;
> +    if (!s || !opaque)
> +        return;
> +    caps = *(AVDeviceCapabilitiesQuery **) opaque;
> +    if (!caps)
>           return;
>       av_assert0(s->iformat || s->oformat);
>       if (s->iformat) {
>           if (s->iformat->free_device_capabilities)
> -            s->iformat->free_device_capabilities(s, *caps);
> +            s->iformat->free_device_capabilities(s, caps);
>       } else {
>           if (s->oformat->free_device_capabilities)
> -            s->oformat->free_device_capabilities(s, *caps);
> +            s->oformat->free_device_capabilities(s, caps);
>       }
> -    av_freep(caps);
> +    av_freep(opaque);
>   }
>   
>   int avdevice_list_devices(AVFormatContext *s, AVDeviceInfoList **device_list)
> diff --git a/libavdevice/avdevice.h b/libavdevice/avdevice.h
> index 7c5e77df00..389ac0b5f2 100644
> --- a/libavdevice/avdevice.h
> +++ b/libavdevice/avdevice.h
> @@ -401,40 +401,15 @@ int avdevice_dev_to_app_control_message(struct AVFormatContext *s,
>    * @endcode
>    */
>   
> -/**
> - * Structure describes device capabilities.
> - *
> - * It is used by devices in conjunction with av_device_capabilities AVOption table
> - * to implement capabilities probing API based on AVOption API. Should not be used directly.
> - */
> -typedef struct AVDeviceCapabilitiesQuery {
> -    const AVClass *av_class;
> -    AVFormatContext *device_context;
> -    enum AVCodecID codec;
> -    enum AVSampleFormat sample_format;
> -    enum AVPixelFormat pixel_format;
> -    int sample_rate;
> -    int channels;
> -    int64_t channel_layout;
> -    int window_width;
> -    int window_height;
> -    int frame_width;
> -    int frame_height;
> -    AVRational fps;
> -} AVDeviceCapabilitiesQuery;

Instead of removing the struct altogether, just keep its typedef here.
That way the functions below and any libavformat struct can still use 
AVDeviceCapabilitiesQuery as an incomplete type.

So in short:

typedef struct AVDeviceCapabilitiesQuery AVDeviceCapabilitiesQuery;

This is nonetheless technically an API break, but since nothing used 
this stuff, i guess we could make an exception. Not sure what other 
developers think.

I assume you intend to also undo the deprecation?

> -
> -/**
> - * AVOption table used by devices to implement device capabilities API. Should not be used by a user.
> - */
> -extern const AVOption av_device_capabilities[];
> -
>   /**
>    * Initialize capabilities probing API based on AVOption API.
>    *
>    * avdevice_capabilities_free() must be called when query capabilities API is
>    * not used anymore.
> - *
> - * @param[out] caps      Device capabilities data. Pointer to a NULL pointer must be passed.
> + *
> + * @param[out] opaque    A pointer where the capabilities API state will be stored. Pointer
> + *                       to a NULL pointer must be passed and caller must not touch this in
> + *                       any way.
>    * @param s              Context of the device.
>    * @param device_options An AVDictionary filled with device-private options.
>    *                       On return this parameter will be destroyed and replaced with a dict
> @@ -445,16 +420,17 @@ extern const AVOption av_device_capabilities[];
>    *
>    * @return >= 0 on success, negative otherwise.
>    */
> -int avdevice_capabilities_create(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s,
> +int avdevice_capabilities_create(void **opaque, AVFormatContext *s,
>                                    AVDictionary **device_options);
>   
>   /**
>    * Free resources created by avdevice_capabilities_create()
>    *
> - * @param caps Device capabilities data to be freed.
> - * @param s    Context of the device.
> + * @param[out] opaque   Pointer to be freed that was returned from
> + *                      avdevice_capabilities_create.
> + * @param s             Context of the device.
>    */
> -void avdevice_capabilities_free(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s);
> +void avdevice_capabilities_free(void **opaque, AVFormatContext *s);
>   
>   /**
>    * Structure describes basic parameters of the device.
> diff --git a/libavdevice/internal.h b/libavdevice/internal.h
> index 67c90e1f87..fe4be64ee7 100644
> --- a/libavdevice/internal.h
> +++ b/libavdevice/internal.h
> @@ -20,9 +20,42 @@
>   #define AVDEVICE_INTERNAL_H
>   
>   #include "libavformat/avformat.h"
> +#include "libavcodec/codec_id.h"
> +#include "libavutil/log.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/pixfmt.h"
> +#include "libavutil/rational.h"
> +#include "libavutil/samplefmt.h"
>   
>   av_warn_unused_result
>   int ff_alloc_input_device_context(struct AVFormatContext **avctx, const AVInputFormat *iformat,
>                                     const char *format);
>   
> +/**
> + * Structure describes device capabilities.
> + *
> + * It is used by devices in conjunction with av_device_capabilities AVOption table
> + * to implement capabilities probing API based on AVOption API. Should not be used directly.
> + */
> +typedef struct AVDeviceCapabilitiesQuery {
> +    const AVClass *av_class;
> +    AVFormatContext *device_context;
> +    enum AVCodecID codec;
> +    enum AVSampleFormat sample_format;
> +    enum AVPixelFormat pixel_format;
> +    int sample_rate;
> +    int channels;
> +    int64_t channel_layout;
> +    int window_width;
> +    int window_height;
> +    int frame_width;
> +    int frame_height;
> +    AVRational fps;
> +} AVDeviceCapabilitiesQuery;

struct AVDeviceCapabilitiesQuery {
     [...]
};

> +
> +/**
> + * AVOption table used by devices to implement device capabilities API. Should not be used by a user.
> + */
> +extern const AVOption av_device_capabilities[];
> +
>   #endif
> diff --git a/libavdevice/version.h b/libavdevice/version.h
> index 0381d6cd0d..53af6fa0d0 100644
> --- a/libavdevice/version.h
> +++ b/libavdevice/version.h
> @@ -28,7 +28,7 @@
>   #include "libavutil/version.h"
>   
>   #define LIBAVDEVICE_VERSION_MAJOR  59
> -#define LIBAVDEVICE_VERSION_MINOR   2
> +#define LIBAVDEVICE_VERSION_MINOR   3
>   #define LIBAVDEVICE_VERSION_MICRO 100
>   
>   #define LIBAVDEVICE_VERSION_INT AV_VERSION_INT(LIBAVDEVICE_VERSION_MAJOR, \
>
Andreas Rheinhardt June 7, 2021, 11:54 p.m. UTC | #2
Diederick Niehorster:
> NB: will break build, makes needed corresponding changes to avformat.
> 

Then said changes should be part of this patch. Patches should be
logically self-contained and atomic; this is not the same as splitting
by file/library.

But that is a moot point: James already told you a better way to fix this.

> Signed-off-by: Diederick Niehorster <dcnieho@gmail.com>
> ---
>  libavdevice/avdevice.c | 34 ++++++++++++++++++++--------------
>  libavdevice/avdevice.h | 42 +++++++++---------------------------------
>  libavdevice/internal.h | 33 +++++++++++++++++++++++++++++++++
>  libavdevice/version.h  |  2 +-
>  4 files changed, 63 insertions(+), 48 deletions(-)
> 
> diff --git a/libavdevice/avdevice.c b/libavdevice/avdevice.c
> index d73d36e1dd..b521516ff5 100644
> --- a/libavdevice/avdevice.c
> +++ b/libavdevice/avdevice.c
> @@ -96,51 +96,57 @@ int avdevice_dev_to_app_control_message(struct AVFormatContext *s, enum AVDevToA
>      return s->control_message_cb(s, type, data, data_size);
>  }
>  
> -int avdevice_capabilities_create(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s,
> +int avdevice_capabilities_create(void** opaque, AVFormatContext *s,
>                                   AVDictionary **device_options)
>  {
>      int ret;
> +    AVDeviceCapabilitiesQuery *caps = NULL;
>      av_assert0(s);
> -    av_assert0(caps);
> +    av_assert0(opaque);
>      av_assert0(s->iformat || s->oformat);
> +    *opaque = NULL;
>      if ((s->oformat && !s->oformat->create_device_capabilities) ||
>          (s->iformat && !s->iformat->create_device_capabilities)) {
> -        *caps = NULL;
>          return AVERROR(ENOSYS);
>      }
> -    *caps = av_mallocz(sizeof(AVDeviceCapabilitiesQuery));
> -    if (!(*caps))
> +    *opaque = caps = av_mallocz(sizeof(AVDeviceCapabilitiesQuery));
> +    if (!caps)
>          return AVERROR(ENOMEM);
> -    (*caps)->device_context = s;
> +    caps->device_context = s;
>      if (((ret = av_opt_set_dict(s->priv_data, device_options)) < 0))
>          goto fail;
>      if (s->iformat) {
> -        if ((ret = s->iformat->create_device_capabilities(s, *caps)) < 0)
> +        if ((ret = s->iformat->create_device_capabilities(s, caps)) < 0)
>              goto fail;
>      } else {
> -        if ((ret = s->oformat->create_device_capabilities(s, *caps)) < 0)
> +        if ((ret = s->oformat->create_device_capabilities(s, caps)) < 0)
>              goto fail;
>      }
> -    av_opt_set_defaults(*caps);
> +    av_opt_set_defaults(caps);
>      return 0;
>    fail:
>      av_freep(caps);
> +    *opaque = NULL;
>      return ret;
>  }
>  
> -void avdevice_capabilities_free(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s)
> +void avdevice_capabilities_free(void **opaque, AVFormatContext *s)
>  {
> -    if (!s || !caps || !(*caps))
> +    AVDeviceCapabilitiesQuery *caps;
> +    if (!s || !opaque)
> +        return;
> +    caps = *(AVDeviceCapabilitiesQuery **) opaque;
> +    if (!caps)
>          return;
>      av_assert0(s->iformat || s->oformat);
>      if (s->iformat) {
>          if (s->iformat->free_device_capabilities)
> -            s->iformat->free_device_capabilities(s, *caps);
> +            s->iformat->free_device_capabilities(s, caps);
>      } else {
>          if (s->oformat->free_device_capabilities)
> -            s->oformat->free_device_capabilities(s, *caps);
> +            s->oformat->free_device_capabilities(s, caps);
>      }
> -    av_freep(caps);
> +    av_freep(opaque);
>  }
>  
>  int avdevice_list_devices(AVFormatContext *s, AVDeviceInfoList **device_list)
> diff --git a/libavdevice/avdevice.h b/libavdevice/avdevice.h
> index 7c5e77df00..389ac0b5f2 100644
> --- a/libavdevice/avdevice.h
> +++ b/libavdevice/avdevice.h
> @@ -401,40 +401,15 @@ int avdevice_dev_to_app_control_message(struct AVFormatContext *s,
>   * @endcode
>   */
>  
> -/**
> - * Structure describes device capabilities.
> - *
> - * It is used by devices in conjunction with av_device_capabilities AVOption table
> - * to implement capabilities probing API based on AVOption API. Should not be used directly.
> - */
> -typedef struct AVDeviceCapabilitiesQuery {
> -    const AVClass *av_class;
> -    AVFormatContext *device_context;
> -    enum AVCodecID codec;
> -    enum AVSampleFormat sample_format;
> -    enum AVPixelFormat pixel_format;
> -    int sample_rate;
> -    int channels;
> -    int64_t channel_layout;
> -    int window_width;
> -    int window_height;
> -    int frame_width;
> -    int frame_height;
> -    AVRational fps;
> -} AVDeviceCapabilitiesQuery;
> -
> -/**
> - * AVOption table used by devices to implement device capabilities API. Should not be used by a user.
> - */
> -extern const AVOption av_device_capabilities[];
> -
>  /**
>   * Initialize capabilities probing API based on AVOption API.
>   *
>   * avdevice_capabilities_free() must be called when query capabilities API is
>   * not used anymore.
> - *
> - * @param[out] caps      Device capabilities data. Pointer to a NULL pointer must be passed.
> + * 
> + * @param[out] opaque    A pointer where the capabilities API state will be stored. Pointer
> + *                       to a NULL pointer must be passed and caller must not touch this in
> + *                       any way.
>   * @param s              Context of the device.
>   * @param device_options An AVDictionary filled with device-private options.
>   *                       On return this parameter will be destroyed and replaced with a dict
> @@ -445,16 +420,17 @@ extern const AVOption av_device_capabilities[];
>   *
>   * @return >= 0 on success, negative otherwise.
>   */
> -int avdevice_capabilities_create(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s,
> +int avdevice_capabilities_create(void **opaque, AVFormatContext *s,
>                                   AVDictionary **device_options);
>  
>  /**
>   * Free resources created by avdevice_capabilities_create()
>   *
> - * @param caps Device capabilities data to be freed.
> - * @param s    Context of the device.
> + * @param[out] opaque   Pointer to be freed that was returned from
> + *                      avdevice_capabilities_create.
> + * @param s             Context of the device.
>   */
> -void avdevice_capabilities_free(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s);
> +void avdevice_capabilities_free(void **opaque, AVFormatContext *s);
>  
>  /**
>   * Structure describes basic parameters of the device.
> diff --git a/libavdevice/internal.h b/libavdevice/internal.h
> index 67c90e1f87..fe4be64ee7 100644
> --- a/libavdevice/internal.h
> +++ b/libavdevice/internal.h
> @@ -20,9 +20,42 @@
>  #define AVDEVICE_INTERNAL_H
>  
>  #include "libavformat/avformat.h"
> +#include "libavcodec/codec_id.h"
> +#include "libavutil/log.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/pixfmt.h"
> +#include "libavutil/rational.h"
> +#include "libavutil/samplefmt.h"

We prefer the order libavutil-libavcodec-libavformat (the reverse of
linking order).

>  
>  av_warn_unused_result
>  int ff_alloc_input_device_context(struct AVFormatContext **avctx, const AVInputFormat *iformat,
>                                    const char *format);
>  
> +/**
> + * Structure describes device capabilities.
> + *
> + * It is used by devices in conjunction with av_device_capabilities AVOption table
> + * to implement capabilities probing API based on AVOption API. Should not be used directly.
> + */
> +typedef struct AVDeviceCapabilitiesQuery {
> +    const AVClass *av_class;
> +    AVFormatContext *device_context;
> +    enum AVCodecID codec;
> +    enum AVSampleFormat sample_format;
> +    enum AVPixelFormat pixel_format;
> +    int sample_rate;
> +    int channels;
> +    int64_t channel_layout;
> +    int window_width;
> +    int window_height;
> +    int frame_width;
> +    int frame_height;
> +    AVRational fps;
> +} AVDeviceCapabilitiesQuery;
> +
> +/**
> + * AVOption table used by devices to implement device capabilities API. Should not be used by a user.

That last part is superfluous, as (external) users by definition have no
business looking into internal.h.

> + */
> +extern const AVOption av_device_capabilities[];

Don't use this name: av_* is our namespace for public symbols. Our
linker script makes sure that all symbols that start with av_* are
exported; whether they are in a public header is irrelevant.
Not exporting the symbol in the first place also solves the problem of
users using it despite it not being part of the API.
(I btw don't know whether said linker script is used on Windows.)

> +
>  #endif
> diff --git a/libavdevice/version.h b/libavdevice/version.h
> index 0381d6cd0d..53af6fa0d0 100644
> --- a/libavdevice/version.h
> +++ b/libavdevice/version.h
> @@ -28,7 +28,7 @@
>  #include "libavutil/version.h"
>  
>  #define LIBAVDEVICE_VERSION_MAJOR  59
> -#define LIBAVDEVICE_VERSION_MINOR   2
> +#define LIBAVDEVICE_VERSION_MINOR   3
>  #define LIBAVDEVICE_VERSION_MICRO 100
>  
>  #define LIBAVDEVICE_VERSION_INT AV_VERSION_INT(LIBAVDEVICE_VERSION_MAJOR, \
>
Andreas Rheinhardt June 8, 2021, 12:01 a.m. UTC | #3
James Almer:
> On 6/7/2021 8:04 PM, Diederick Niehorster wrote:
>> NB: will break build, makes needed corresponding changes to avformat.
>>
>> Signed-off-by: Diederick Niehorster <dcnieho@gmail.com>
>> ---
>>   libavdevice/avdevice.c | 34 ++++++++++++++++++++--------------
>>   libavdevice/avdevice.h | 42 +++++++++---------------------------------
>>   libavdevice/internal.h | 33 +++++++++++++++++++++++++++++++++
>>   libavdevice/version.h  |  2 +-
>>   4 files changed, 63 insertions(+), 48 deletions(-)
>>
>> diff --git a/libavdevice/avdevice.c b/libavdevice/avdevice.c
>> index d73d36e1dd..b521516ff5 100644
>> --- a/libavdevice/avdevice.c
>> +++ b/libavdevice/avdevice.c
>> @@ -96,51 +96,57 @@ int avdevice_dev_to_app_control_message(struct
>> AVFormatContext *s, enum AVDevToA
>>       return s->control_message_cb(s, type, data, data_size);
>>   }
>>   -int avdevice_capabilities_create(AVDeviceCapabilitiesQuery **caps,
>> AVFormatContext *s,
>> +int avdevice_capabilities_create(void** opaque, AVFormatContext *s,
>>                                    AVDictionary **device_options)
>>   {
>>       int ret;
>> +    AVDeviceCapabilitiesQuery *caps = NULL;
>>       av_assert0(s);
>> -    av_assert0(caps);
>> +    av_assert0(opaque);
>>       av_assert0(s->iformat || s->oformat);
>> +    *opaque = NULL;
>>       if ((s->oformat && !s->oformat->create_device_capabilities) ||
>>           (s->iformat && !s->iformat->create_device_capabilities)) {
>> -        *caps = NULL;
>>           return AVERROR(ENOSYS);
>>       }
>> -    *caps = av_mallocz(sizeof(AVDeviceCapabilitiesQuery));
>> -    if (!(*caps))
>> +    *opaque = caps = av_mallocz(sizeof(AVDeviceCapabilitiesQuery));
>> +    if (!caps)
>>           return AVERROR(ENOMEM);
>> -    (*caps)->device_context = s;
>> +    caps->device_context = s;
>>       if (((ret = av_opt_set_dict(s->priv_data, device_options)) < 0))
>>           goto fail;
>>       if (s->iformat) {
>> -        if ((ret = s->iformat->create_device_capabilities(s, *caps))
>> < 0)
>> +        if ((ret = s->iformat->create_device_capabilities(s, caps)) < 0)
>>               goto fail;
>>       } else {
>> -        if ((ret = s->oformat->create_device_capabilities(s, *caps))
>> < 0)
>> +        if ((ret = s->oformat->create_device_capabilities(s, caps)) < 0)
>>               goto fail;
>>       }
>> -    av_opt_set_defaults(*caps);
>> +    av_opt_set_defaults(caps);
>>       return 0;
>>     fail:
>>       av_freep(caps);
>> +    *opaque = NULL;
>>       return ret;
>>   }
>>   -void avdevice_capabilities_free(AVDeviceCapabilitiesQuery **caps,
>> AVFormatContext *s)
>> +void avdevice_capabilities_free(void **opaque, AVFormatContext *s)
>>   {
>> -    if (!s || !caps || !(*caps))
>> +    AVDeviceCapabilitiesQuery *caps;
>> +    if (!s || !opaque)
>> +        return;
>> +    caps = *(AVDeviceCapabilitiesQuery **) opaque;
>> +    if (!caps)
>>           return;
>>       av_assert0(s->iformat || s->oformat);
>>       if (s->iformat) {
>>           if (s->iformat->free_device_capabilities)
>> -            s->iformat->free_device_capabilities(s, *caps);
>> +            s->iformat->free_device_capabilities(s, caps);
>>       } else {
>>           if (s->oformat->free_device_capabilities)
>> -            s->oformat->free_device_capabilities(s, *caps);
>> +            s->oformat->free_device_capabilities(s, caps);
>>       }
>> -    av_freep(caps);
>> +    av_freep(opaque);
>>   }
>>     int avdevice_list_devices(AVFormatContext *s, AVDeviceInfoList
>> **device_list)
>> diff --git a/libavdevice/avdevice.h b/libavdevice/avdevice.h
>> index 7c5e77df00..389ac0b5f2 100644
>> --- a/libavdevice/avdevice.h
>> +++ b/libavdevice/avdevice.h
>> @@ -401,40 +401,15 @@ int avdevice_dev_to_app_control_message(struct
>> AVFormatContext *s,
>>    * @endcode
>>    */
>>   -/**
>> - * Structure describes device capabilities.
>> - *
>> - * It is used by devices in conjunction with av_device_capabilities
>> AVOption table
>> - * to implement capabilities probing API based on AVOption API.
>> Should not be used directly.
>> - */
>> -typedef struct AVDeviceCapabilitiesQuery {
>> -    const AVClass *av_class;
>> -    AVFormatContext *device_context;
>> -    enum AVCodecID codec;
>> -    enum AVSampleFormat sample_format;
>> -    enum AVPixelFormat pixel_format;
>> -    int sample_rate;
>> -    int channels;
>> -    int64_t channel_layout;
>> -    int window_width;
>> -    int window_height;
>> -    int frame_width;
>> -    int frame_height;
>> -    AVRational fps;
>> -} AVDeviceCapabilitiesQuery;
> 
> Instead of removing the struct altogether, just keep its typedef here.
> That way the functions below and any libavformat struct can still use
> AVDeviceCapabilitiesQuery as an incomplete type.
> 
> So in short:
> 
> typedef struct AVDeviceCapabilitiesQuery AVDeviceCapabilitiesQuery;
> 
> This is nonetheless technically an API break, but since nothing used
> this stuff, i guess we could make an exception. Not sure what other
> developers think.
> 

See the "Should not be used directly." above. It was not allowed to be
used by a user, ergo it is not an API break; same goes for
av_device_capabilites (which I somehow forgot to remove during the
bump). Given that they are not allowed to be used, it never made sense
to have them public.

- Andreas
Diederick C. Niehorster June 8, 2021, 5:59 a.m. UTC | #4
On Tue, Jun 8, 2021 at 1:26 AM James Almer <jamrial@gmail.com> wrote:

> On 6/7/2021 8:04 PM, Diederick Niehorster wrote:
> Instead of removing the struct altogether, just keep its typedef here.
> That way the functions below and any libavformat struct can still use
> AVDeviceCapabilitiesQuery as an incomplete type.
>
> So in short:
>
> typedef struct AVDeviceCapabilitiesQuery AVDeviceCapabilitiesQuery;
>

Will do.


> This is nonetheless technically an API break, but since nothing used
> this stuff, i guess we could make an exception. Not sure what other
> developers think.
>

That was my thinking, and I see Andreas agrees with this too, good.


>
> I assume you intend to also undo the deprecation?
>

Yes, thats patch 19/35 in this series.
Diederick C. Niehorster June 8, 2021, 6:01 a.m. UTC | #5
On Tue, Jun 8, 2021 at 1:54 AM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> Diederick Niehorster:
> > NB: will break build, makes needed corresponding changes to avformat.
> >
>
> Then said changes should be part of this patch. Patches should be
> logically self-contained and atomic; this is not the same as splitting
> by file/library.
>
> But that is a moot point: James already told you a better way to fix this.
>

Made sense. But indeed, James' way of doing it obviates that next patch
changing avformat.h


> > --- a/libavdevice/internal.h
> > +++ b/libavdevice/internal.h
>
> We prefer the order libavutil-libavcodec-libavformat (the reverse of
> linking order).
>

fixed.


> > + * AVOption table used by devices to implement device capabilities API.
> Should not be used by a user.
>
> That last part is superfluous, as (external) users by definition have no
> business looking into internal.h.
>

Ah yes, copy paste.. Fixed.


> > + */
> > +extern const AVOption av_device_capabilities[];
>
> Don't use this name: av_* is our namespace for public symbols. Our
> linker script makes sure that all symbols that start with av_* are
> exported; whether they are in a public header is irrelevant.
> Not exporting the symbol in the first place also solves the problem of
> users using it despite it not being part of the API.
> (I btw don't know whether said linker script is used on Windows.)
>

I changed it to a ff_ prefix. Is that correct?
Andreas Rheinhardt June 8, 2021, 10:46 a.m. UTC | #6
Diederick C. Niehorster:
> On Tue, Jun 8, 2021 at 1:54 AM Andreas Rheinhardt <
> andreas.rheinhardt@outlook.com> wrote:
> 
>> Diederick Niehorster:
>>> NB: will break build, makes needed corresponding changes to avformat.
>>>
>>
>> Then said changes should be part of this patch. Patches should be
>> logically self-contained and atomic; this is not the same as splitting
>> by file/library.
>>
>> But that is a moot point: James already told you a better way to fix this.
>>
> 
> Made sense. But indeed, James' way of doing it obviates that next patch
> changing avformat.h
> 
> 
>>> --- a/libavdevice/internal.h
>>> +++ b/libavdevice/internal.h
>>
>> We prefer the order libavutil-libavcodec-libavformat (the reverse of
>> linking order).
>>
> 
> fixed.
> 
> 
>>> + * AVOption table used by devices to implement device capabilities API.
>> Should not be used by a user.
>>
>> That last part is superfluous, as (external) users by definition have no
>> business looking into internal.h.
>>
> 
> Ah yes, copy paste.. Fixed.
> 
> 
>>> + */
>>> +extern const AVOption av_device_capabilities[];
>>
>> Don't use this name: av_* is our namespace for public symbols. Our
>> linker script makes sure that all symbols that start with av_* are
>> exported; whether they are in a public header is irrelevant.
>> Not exporting the symbol in the first place also solves the problem of
>> users using it despite it not being part of the API.
>> (I btw don't know whether said linker script is used on Windows.)
>>
> 
> I changed it to a ff_ prefix. Is that correct?

Yes (presuming there are (or will eventually be) users of this in
different translation units).

- Andreas
diff mbox series

Patch

diff --git a/libavdevice/avdevice.c b/libavdevice/avdevice.c
index d73d36e1dd..b521516ff5 100644
--- a/libavdevice/avdevice.c
+++ b/libavdevice/avdevice.c
@@ -96,51 +96,57 @@  int avdevice_dev_to_app_control_message(struct AVFormatContext *s, enum AVDevToA
     return s->control_message_cb(s, type, data, data_size);
 }
 
-int avdevice_capabilities_create(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s,
+int avdevice_capabilities_create(void** opaque, AVFormatContext *s,
                                  AVDictionary **device_options)
 {
     int ret;
+    AVDeviceCapabilitiesQuery *caps = NULL;
     av_assert0(s);
-    av_assert0(caps);
+    av_assert0(opaque);
     av_assert0(s->iformat || s->oformat);
+    *opaque = NULL;
     if ((s->oformat && !s->oformat->create_device_capabilities) ||
         (s->iformat && !s->iformat->create_device_capabilities)) {
-        *caps = NULL;
         return AVERROR(ENOSYS);
     }
-    *caps = av_mallocz(sizeof(AVDeviceCapabilitiesQuery));
-    if (!(*caps))
+    *opaque = caps = av_mallocz(sizeof(AVDeviceCapabilitiesQuery));
+    if (!caps)
         return AVERROR(ENOMEM);
-    (*caps)->device_context = s;
+    caps->device_context = s;
     if (((ret = av_opt_set_dict(s->priv_data, device_options)) < 0))
         goto fail;
     if (s->iformat) {
-        if ((ret = s->iformat->create_device_capabilities(s, *caps)) < 0)
+        if ((ret = s->iformat->create_device_capabilities(s, caps)) < 0)
             goto fail;
     } else {
-        if ((ret = s->oformat->create_device_capabilities(s, *caps)) < 0)
+        if ((ret = s->oformat->create_device_capabilities(s, caps)) < 0)
             goto fail;
     }
-    av_opt_set_defaults(*caps);
+    av_opt_set_defaults(caps);
     return 0;
   fail:
     av_freep(caps);
+    *opaque = NULL;
     return ret;
 }
 
-void avdevice_capabilities_free(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s)
+void avdevice_capabilities_free(void **opaque, AVFormatContext *s)
 {
-    if (!s || !caps || !(*caps))
+    AVDeviceCapabilitiesQuery *caps;
+    if (!s || !opaque)
+        return;
+    caps = *(AVDeviceCapabilitiesQuery **) opaque;
+    if (!caps)
         return;
     av_assert0(s->iformat || s->oformat);
     if (s->iformat) {
         if (s->iformat->free_device_capabilities)
-            s->iformat->free_device_capabilities(s, *caps);
+            s->iformat->free_device_capabilities(s, caps);
     } else {
         if (s->oformat->free_device_capabilities)
-            s->oformat->free_device_capabilities(s, *caps);
+            s->oformat->free_device_capabilities(s, caps);
     }
-    av_freep(caps);
+    av_freep(opaque);
 }
 
 int avdevice_list_devices(AVFormatContext *s, AVDeviceInfoList **device_list)
diff --git a/libavdevice/avdevice.h b/libavdevice/avdevice.h
index 7c5e77df00..389ac0b5f2 100644
--- a/libavdevice/avdevice.h
+++ b/libavdevice/avdevice.h
@@ -401,40 +401,15 @@  int avdevice_dev_to_app_control_message(struct AVFormatContext *s,
  * @endcode
  */
 
-/**
- * Structure describes device capabilities.
- *
- * It is used by devices in conjunction with av_device_capabilities AVOption table
- * to implement capabilities probing API based on AVOption API. Should not be used directly.
- */
-typedef struct AVDeviceCapabilitiesQuery {
-    const AVClass *av_class;
-    AVFormatContext *device_context;
-    enum AVCodecID codec;
-    enum AVSampleFormat sample_format;
-    enum AVPixelFormat pixel_format;
-    int sample_rate;
-    int channels;
-    int64_t channel_layout;
-    int window_width;
-    int window_height;
-    int frame_width;
-    int frame_height;
-    AVRational fps;
-} AVDeviceCapabilitiesQuery;
-
-/**
- * AVOption table used by devices to implement device capabilities API. Should not be used by a user.
- */
-extern const AVOption av_device_capabilities[];
-
 /**
  * Initialize capabilities probing API based on AVOption API.
  *
  * avdevice_capabilities_free() must be called when query capabilities API is
  * not used anymore.
- *
- * @param[out] caps      Device capabilities data. Pointer to a NULL pointer must be passed.
+ * 
+ * @param[out] opaque    A pointer where the capabilities API state will be stored. Pointer
+ *                       to a NULL pointer must be passed and caller must not touch this in
+ *                       any way.
  * @param s              Context of the device.
  * @param device_options An AVDictionary filled with device-private options.
  *                       On return this parameter will be destroyed and replaced with a dict
@@ -445,16 +420,17 @@  extern const AVOption av_device_capabilities[];
  *
  * @return >= 0 on success, negative otherwise.
  */
-int avdevice_capabilities_create(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s,
+int avdevice_capabilities_create(void **opaque, AVFormatContext *s,
                                  AVDictionary **device_options);
 
 /**
  * Free resources created by avdevice_capabilities_create()
  *
- * @param caps Device capabilities data to be freed.
- * @param s    Context of the device.
+ * @param[out] opaque   Pointer to be freed that was returned from
+ *                      avdevice_capabilities_create.
+ * @param s             Context of the device.
  */
-void avdevice_capabilities_free(AVDeviceCapabilitiesQuery **caps, AVFormatContext *s);
+void avdevice_capabilities_free(void **opaque, AVFormatContext *s);
 
 /**
  * Structure describes basic parameters of the device.
diff --git a/libavdevice/internal.h b/libavdevice/internal.h
index 67c90e1f87..fe4be64ee7 100644
--- a/libavdevice/internal.h
+++ b/libavdevice/internal.h
@@ -20,9 +20,42 @@ 
 #define AVDEVICE_INTERNAL_H
 
 #include "libavformat/avformat.h"
+#include "libavcodec/codec_id.h"
+#include "libavutil/log.h"
+#include "libavutil/opt.h"
+#include "libavutil/pixfmt.h"
+#include "libavutil/rational.h"
+#include "libavutil/samplefmt.h"
 
 av_warn_unused_result
 int ff_alloc_input_device_context(struct AVFormatContext **avctx, const AVInputFormat *iformat,
                                   const char *format);
 
+/**
+ * Structure describes device capabilities.
+ *
+ * It is used by devices in conjunction with av_device_capabilities AVOption table
+ * to implement capabilities probing API based on AVOption API. Should not be used directly.
+ */
+typedef struct AVDeviceCapabilitiesQuery {
+    const AVClass *av_class;
+    AVFormatContext *device_context;
+    enum AVCodecID codec;
+    enum AVSampleFormat sample_format;
+    enum AVPixelFormat pixel_format;
+    int sample_rate;
+    int channels;
+    int64_t channel_layout;
+    int window_width;
+    int window_height;
+    int frame_width;
+    int frame_height;
+    AVRational fps;
+} AVDeviceCapabilitiesQuery;
+
+/**
+ * AVOption table used by devices to implement device capabilities API. Should not be used by a user.
+ */
+extern const AVOption av_device_capabilities[];
+
 #endif
diff --git a/libavdevice/version.h b/libavdevice/version.h
index 0381d6cd0d..53af6fa0d0 100644
--- a/libavdevice/version.h
+++ b/libavdevice/version.h
@@ -28,7 +28,7 @@ 
 #include "libavutil/version.h"
 
 #define LIBAVDEVICE_VERSION_MAJOR  59
-#define LIBAVDEVICE_VERSION_MINOR   2
+#define LIBAVDEVICE_VERSION_MINOR   3
 #define LIBAVDEVICE_VERSION_MICRO 100
 
 #define LIBAVDEVICE_VERSION_INT AV_VERSION_INT(LIBAVDEVICE_VERSION_MAJOR, \