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

[Lukasz M]

On 9 February 2014 10:01, Nicolas George <george at nsup.org> wrote:

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

Done


>
> > +    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.


Yes, it is a bug. It didn't crash because I tested on opengl that have just
one element and it gave correct results in wrong way.


> +    }
> > +    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.
>

I changed it to AVDeviceInfo **devices.
I also don't know if ever another field will be added, but I think it is
safer to leave that possibility.


>
> > +    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.
>

OK, changed


>
> >  } 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.
>

Thanks
Updated patch attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavd-add-list-devices-API.patch
Type: text/x-patch
Size: 5017 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140211/6c901c33/attachment.bin>