[FFmpeg-devel] [PATCH 1/4] lavd: add list devices API

[Nicolas George]

Le nonidi 19 pluviôse, an CCXXII, Lukasz Marek a écrit :
> TODO: minor bumps, APIchange update
> 
> Signed-off-by: Lukasz Marek <lukasz.m.luki at gmail.com>
> ---
>  libavdevice/avdevice.c | 32 ++++++++++++++++++++++++++++++++
>  libavdevice/avdevice.h | 39 +++++++++++++++++++++++++++++++++++++++
>  libavformat/avformat.h | 10 ++++++++++
>  3 files changed, 81 insertions(+)
> 
> diff --git a/libavdevice/avdevice.c b/libavdevice/avdevice.c
> index 51617fb..a418ef8 100644
> --- a/libavdevice/avdevice.c
> +++ b/libavdevice/avdevice.c
> @@ -52,3 +52,35 @@ int avdevice_dev_to_app_control_message(struct AVFormatContext *s, enum AVDevToA
>          return AVERROR(ENOSYS);
>      return s->control_message_cb(s, type, data, data_size);
>  }
> +
> +int avdevice_list_devices(AVFormatContext *s, AVDeviceInfoList **device_list)
> +{
> +    av_assert1(s);
> +    av_assert1(s->oformat || s->iformat);
> +    if ((s->oformat && !s->oformat->get_device_list) ||
> +        (s->iformat && !s->iformat->get_device_list))
> +        return AVERROR(ENOSYS);

Suggestion: mallocate *device_list here, so that the method in the format
does not need to do it itself

> +    if (s->oformat)
> +        return s->oformat->get_device_list(s, (void **)device_list);
> +    else
> +        return s->iformat->get_device_list(s, (void **)device_list);
> +}
> +
> +void avdevice_free_list_devices(AVDeviceInfoList **device_list)
> +{
> +    AVDeviceInfoList *list;
> +    AVDeviceInfo *dev;
> +    int i;
> +
> +    if (!device_list || !(*device_list))
> +        return;
> +    list = *device_list;
> +
> +    for (i = 0; i < list->nb_devices; i++) {
> +        dev = &list->devices[i];
> +        av_free(dev->device_name);
> +        av_free(dev->device_description);

> +        av_free(dev);

Are you sure? It looks to me like list->devices is a single array, it should
be freed after the loop. But see below for more about that.

> +    }
> +    av_freep(device_list);
> +}
> diff --git a/libavdevice/avdevice.h b/libavdevice/avdevice.h
> index a6408ea..2392cac 100644
> --- a/libavdevice/avdevice.h
> +++ b/libavdevice/avdevice.h
> @@ -186,4 +186,43 @@ int avdevice_dev_to_app_control_message(struct AVFormatContext *s,
>                                          enum AVDevToAppMessageType type,
>                                          void *data, size_t data_size);
>  
> +/**
> + * Structure describes basic parameters of the device.
> + */
> +typedef struct AVDeviceInfo {
> +    char *device_name;                   /**< device name, format depends on device */
> +    char *device_description;            /**< human friendly name */
> +} AVDeviceInfo;
> +
> +/**
> + * List of devices.
> + */
> +typedef struct AVDeviceInfoList {

> +    AVDeviceInfo *devices;               /**< list of autodetected devices */

I wonder about this point. On one side, a single array is simple and easy.

On the other side, a single array requires the size of the elements to be
constant; therefore, it forbids adding fields to AVDeviceInfo.

The other possibility is to have "AVDeviceInfo **devices;" point to an array
of pointers to individually allocated structures. I am not sure how likely
it is to need to add fields to AVDeviceInfo.

> +    int nb_devices;                      /**< number of autodetected devices */
> +    int default_device;                  /**< index of default device or -1 if no default */
> +} AVDeviceInfoList;
> +
> +/**
> + * List devices.
> + *
> + * Returns available device names and their parameters.
> + *
> + * @note: Some devices may accept system-dependent device names that cannot be
> + *        autodetected. The list returned by this function cannot be assumed to
> + *        be always completed.
> + *
> + * @param s                device context.
> + * @param[out] device_list list of autodetected devices.
> + * @return count of autodetected devices, negative on error.
> + */
> +int avdevice_list_devices(struct AVFormatContext *s, AVDeviceInfoList **device_list);
> +
> +/**
> + * Convinient function to free result of avdevice_list_devices().
> + *
> + * @param devices device list to be freed.
> + */
> +void avdevice_free_list_devices(AVDeviceInfoList **device_list);
> +
>  #endif /* AVDEVICE_AVDEVICE_H */
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 4d3cc14..dc9aeee 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -460,6 +460,11 @@ typedef struct AVOutputFormat {
>       */
>      int (*control_message)(struct AVFormatContext *s, int type,
>                             void *data, size_t data_size);
> +    /**
> +     * Returns device list with it properties.
> +     * @see avdevice_list_devices() for more details.
> +     */

> +    int (*get_device_list)(struct AVFormatContext *s, void **device_list);

I believe it would be cleaner to pre-declare the structure and use it here
than to type-prune.

>  } AVOutputFormat;
>  /**
>   * @}
> @@ -588,6 +593,11 @@ typedef struct AVInputFormat {
>       * Active streams are all streams that have AVStream.discard < AVDISCARD_ALL.
>       */
>      int (*read_seek2)(struct AVFormatContext *s, int stream_index, int64_t min_ts, int64_t ts, int64_t max_ts, int flags);
> +    /**
> +     * Returns device list with it properties.
> +     * @see avdevice_list_devices() for more details.
> +     */
> +    int (*get_device_list)(struct AVFormatContext *s, void **device_list);
>  } AVInputFormat;
>  /**
>   * @}

Looks very good on the whole, thanks.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140209/79220aac/attachment.asc>