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

Georg Lippitsch georg.lippitsch at gmx.at
Tue Jul 3 23:59:56 CEST 2012


Am 01.07.2012, 01:24 Uhr, schrieb Stefano Sabatini <stefasab at gmail.com>:

>> + at item dvbuffer
>> +Maxiumum size of buffer for incoming data, in frames. For DV, this
>
> Set maximum ...

Fixed

>> +    raw1394handle_t handle;             ///< handle for libraw1394
>
> nit: handle -> raw1394?

Fixed

>> +    pthread_t receive_thread;
>
> Nit++: receive_task_thread;

Fixed


>> +            if (!(errno == EAGAIN || errno == EINTR)) {
>
> nit+: the De-Morgan equivalent (errno != EAGAIN && errno != EINTR)
> looks more readable to me, but that's just a personal opinion

Ignored, since I've copied this from the dvgrab utility, and actually  
didn't see a reason to change.


> Nit: maybe
>      av_log(NULL, AV_LOG_ERROR, "Raw1394 poll error occurred: %s\n",  
> av_err2str(AVERROR(errno)));

I've never seen this error actually happening, so I don't care ...

>
> also I wonder if you can avoid the NULL context.

I could pass the AVFormatContext from somewhere, but I don't know if it's  
save to access from within the thread.
Besides that, I find it questionable adding that many lines of code  
passing the context only for the log messages.


>> +static int iec61883_read_header(AVFormatContext *context)
>> +{
>> +    struct iec61883_data *dv = context->priv_data;
>> +    struct raw1394_portinfo pinf[16];
>
> nit++: pinf -> portinfo?

Also taken from dvgrab.

>
>> +    rom1394_directory rom_dir;
>> +    char *endptr;
>> +    int inport;
>
>> +    int ports;
>
> nit: nb_ports should clarify (ports/port is a bit confusing)

Fixed

>
>
>> +    } else {
>
> please specify in a comment this is the DV case (or with an assert if
> you prefer), will improve readability somewhat

Fixed


>> +        if (result > 0 && ((dv->raw1394_poll.revents & POLLIN)
>> +                           || (dv->raw1394_poll.revents & POLLPRI)))
>> +            raw1394_loop_iterate(dv->handle);
>
> Maybe can be factorized with iec61883_receive_task().

Not completely sure what you mean, but I tried to re-use  
iec61883_receive_task() also in non-thread code. It saves some lines of  
code, but adds some more #ifdef



>> +    { "dvtype", "Override autodetection of DV/HDV", offsetof(struct  
>> iec61883_data, type), AV_OPT_TYPE_INT, {.dbl = IEC61883_AUTO},  
>> IEC61883_AUTO, IEC61883_HDV, AV_OPT_FLAG_DECODING_PARAM, "dvtype" },
>> +    { "auto",   "Auto detect DV/HDV", 0, AV_OPT_TYPE_CONST, {.dbl =  
>> IEC61883_AUTO}, 0, 0, AV_OPT_FLAG_DECODING_PARAM, "dvtype" },
>> +    { "dv",     "Force device being treated as DV device", 0,  
>> AV_OPT_TYPE_CONST, {.dbl = IEC61883_DV},   0, 0,  
>> AV_OPT_FLAG_DECODING_PARAM, "dvtype" },
>> +    { "hdv" ,   "Force device being treated as HDV device", 0,  
>> AV_OPT_TYPE_CONST, {.dbl = IEC61883_HDV},  0, 0,  
>> AV_OPT_FLAG_DECODING_PARAM, "dvtype" },
>> +    { "dvbuffer", "Set queue size (in packets)", offsetof(struct  
>> iec61883_data, max_packets), AV_OPT_TYPE_INT, {.dbl = 0}, 0, INT_MAX,  
>> AV_OPT_FLAG_DECODING_PARAM },
>> +    { NULL },
>> +};
>
> Please no Uppercasing in options

You mean in the description?
Fixed, although DV would be more correct than dv AFAIK...


>
> [...]
>
> Again, add yourself to MAINTAINERS if you wish to maintain this
> device.

Done.


> Apart from that, if there are no more comments from other developers
> and you say the patch is tested enough I suppose it is safe to commit
> it.

I think I've tested it accurately, at least it isn't worse than the old  
dv1394.c


Regards,

Georg
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-FireWire-DV-HDV-input-device-using-libiec61883.patch
Type: text/x-patch
Size: 21020 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120703/11e12353/attachment.bin>


More information about the ffmpeg-devel mailing list