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

Don Moir donmoir at comcast.net
Fri Feb 14 01:28:05 CET 2014


----- Original Message ----- 
From: "Lukasz M" <lukasz.m.luki at gmail.com>
To: "FFmpeg development discussions and patches" <ffmpeg-devel at ffmpeg.org>
Sent: Thursday, February 13, 2014 6:00 PM
Subject: Re: [FFmpeg-devel] [PATCH 1/4] lavd: add list devices API


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

I suppose if user does something dumb like pass in a null context and av_assert1 ends up being defined as av_assert1(cond)((void)0) 
it will crash unepectedly.

#if defined(ASSERT_LEVEL) && ASSERT_LEVEL > 0
#define av_assert1(cond) av_assert0(cond)
#else
#define av_assert1(cond) ((void)0)
#endif

I think it would be better to just return an error code if 's' is null. Similiar problem if both oformat and iformat are null. Don't 
think you really need to crash here but you could use av_assert0 instead.

> 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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel at ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel 



More information about the ffmpeg-devel mailing list