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

[Lukasz M]

On 11 February 2014 14:19, Lukasz M <lukasz.m.luki at gmail.com> wrote:

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

Any further remarks? I don't want to hurry anybody, but if no remarks I
prefer to not wait, because it is a bit blocking me from implementing other
things.