| Message ID | 20210607230414.612-22-dcnieho@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | avdevice (mostly dshow) enhancements | expand |
| 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 |
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, \ >
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, \ >
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
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.
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?
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 --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, \
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(-)