[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