[FFmpeg-devel] FireWire DV/HDV input device using libiec61883
Stefano Sabatini
stefasab at gmail.com
Wed Jul 4 01:49:20 CEST 2012
On date Tuesday 2012-07-03 23:59:56 +0200, Georg Lippitsch encoded:
> 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...
No, I mean that the first word should not be capitalized, but no need
to send another patch for that.
> >[...]
> >
> >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
Assuming there are no other significant changes to the code it should
be fine for me, I'll apply in a few days if I see no more comments
from other devs, thanks so far for your work.
--
FFmpeg = Free & Fabulous Mind-dumbing Pure Efficient Geisha
More information about the ffmpeg-devel
mailing list