[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