[FFmpeg-devel] [PATCH] FireWire DV/HDV input device using libiec61883

[Georg Lippitsch]

Am 24.04.2012, 15:50 Uhr, schrieb Stefano Sabatini <stefasab at gmail.com>:

> Would you mind adding some generic information in doc/indevs.texi
> about the device? Anything which is required to know in order to use
> the device and one or more examples may fit well.

Ok, will submit in the next patch.

> Nit+: inverted order (from system, to libav* library to current dir is
> preferred)

Fixed

>> +#define MOTDCT_SPEC_ID      0x00005068
>
>> +#define IEC61883_AUTO       0
>> +#define IEC61883_DV         1
>> +#define IEC61883_HDV        2
>
> Nit+: this could be an enum

They are used by AVOption, and there always defines are used IIUC. Or am I  
wrong here?

>> + * For DV, one packet correspondets exactly to one frame.
>
> corresponds

Fixed

> nit+: here and below, non complete sentences should not be
> Capitalized, like in:
>
> This is a complete sentence.
> an incomplete sentence

Fixed

> typo: quueue

Fixed

>> +    int isHDV;                          ///< Before connecting, find  
>> out if DV/HDV
>
> Nit: camelStyle is avoided in libav*, snake_style is preferred

Fixed

> Also if I understand it correctly this is not a binary field, but a
> type (auto/DV/HDV) (may slightly help understandibility)

It is actually used twice: Once by AVOption, which sets it to either  
auto/DV/HDV.
After detection in read_header, it is used as binary.
This might be confusing, but I didn't want to introduce an extra variable.  
Or would you prefer that?

>> +static int iec61883_callback_dv(unsigned char *data, int length,
>> +                                 int complete, void *callback_data)
>
> weird indent

Fixed


>> +        packet->buf = av_malloc(length);
>
> missing NULL check?

All NULL checks fixed.


>> +    while(dv->queue_first) {
>
> nit++: while_(

Fixed

>> +    /* Select first AV/C tape reccorder player node */
>
> typo: reccorder

Fixed

>> +    while ((size = dv->parse_queue(dv, pkt)) == -1) {
>> +        while ((result = poll(&dv->raw1394_poll, 1, 200)) < 0) {
>> +            if (!(errno == EAGAIN || errno == EINTR)) {
>
>> +                av_log(context, AV_LOG_ERROR, "Raw1394 poll.\n");
>
> "Raw1394 poll error occurred.\n"

Fixed

>
> Or you could store the error and print the corresponding description
> to the output, I mean:
> ret = ...
> if ((!(ret == ...)
>    av_log(context, ERROR, "Raw1394 poll error occurred: %s\n",  
> strerror_r(errbuf, errbuf_size, err));
>
> but maybe overkill

I think it's overkill, particularly because I've never seen this actually  
happen.

>
>> +    { "auto",   "", 0, AV_OPT_TYPE_CONST, {.dbl = IEC61883_AUTO}, 0,  
>> 0, AV_OPT_FLAG_DECODING_PARAM, "dvtype" },
>> +    { "dv",     "", 0, AV_OPT_TYPE_CONST, {.dbl = IEC61883_DV},   0,  
>> 0, AV_OPT_FLAG_DECODING_PARAM, "dvtype" },
>> +    { "hdv" ,   "", 0, AV_OPT_TYPE_CONST, {.dbl = IEC61883_HDV},  0,  
>> 0, AV_OPT_FLAG_DECODING_PARAM, "dvtype" },
>
> please add a short description

Fixed

>
>> +    { "hdvbuffer", "For HDV, buffer size (in packets) used by  
>> libiec61883", offsetof(struct iec61883_data, buffersize),  
>> AV_OPT_TYPE_INT, {.dbl = 0}, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM },
>
> Nit: "for HDV, set buffer size ..."

Fixed


>> +AVInputFormat ff_iec61883_demuxer = {
>> +    .name           = "iec61883",
>> +    .long_name      = NULL_IF_CONFIG_SMALL("libiec61883 (new DV1394)  
>> A/V grab"),
>
> Nit, subjective: A/V grab => A/V input device


Fixed


Regards,

Georg
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Fix-typos-and-some-nits-in-libiec61883-input-device.patch
Type: text/x-patch
Size: 11038 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120425/a2431cdd/attachment.bin>