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

Tue May 8 01:45:11 CEST 2012

On date Saturday 2012-05-05 20:50:20 +0200, Georg Lippitsch encoded:
> Am 02.05.2012, 02:00 Uhr, schrieb Stefano Sabatini <stefasab at gmail.com>:
> 
> >On date Tuesday 2012-05-01 11:43:18 +0200, Georg Lippitsch encoded:
> >>+    inport = strtol(context->filename, &endptr, 10);
> >>+    if (endptr != context->filename && *endptr == '\0') {
> >>+        av_log(context, AV_LOG_INFO, "Selecting IEEE1394 port:
> >>%d\n", inport);
> >>+        j = inport;
> >>+        ports = inport + 1;
> >>+    }
> >>+
> >
> >What about to explicitely fail in case the string can't be parsed? I
> >mean you could define a constant like "auto" and abort if the string
> >can't be parsed as a number. This should prevent bogus input like
> >"1f" or "1 ".
> 

> Actually I really do NOT want to fail here in any case. Any bogus
> input should trigger auto-detection, because I don't expect people
> to read documentation and search what they should enter here. If
> they enter anything, then the port is auto detected. Only if they
> enter a valid port number, this overrides auto-detection.

Well in case of error you can state something like:
|Invalid input "foobar", you should specify "auto" for auto-detection
|or the port number.

The rationale is that in the (unlikely) case of typo the error is
immediately recognized and you save a few headaches, and you instruct
the user about the syntax without she having to shove manuals. Also
you can extend it later (like adding "best", "first", "last" or
whatever) without breaking syntax.

> 
> >Again I find this quite confusing, since the same field is used for
> >two different purposes. What I suggest:
> >
> >    if (dv->type == AUTO) {
> >        response = avc1394_transaction(dv->handle, dv->node,
> >                                       AVC1394_CTYPE_STATUS |

> >AVC1394_SUBUNIT_TYPE_TAPE_RECORDER |
> >                                       AVC1394_SUBUNIT_ID_0 |

> >AVC1394_VCR_COMMAND_OUTPUT_SIGNAL_MODE |
> >                                       0xFF, 2);
> >        response = AVC1394_GET_OPERAND0(response);
> >        dv->type = (response == 0x10 || response == 0x90 ||
> >response == 0x1A || response == 0x9A) ? HDV : DV;
> >    }
> 
> Indeed, that sounds reasonable. Fixed in attached patch.
> 
> 
> >typo: prohibited
> 
> Fixed.
> 
> 
> >Note: you can squeeze the two patches into one.
> 
> Done.
> 
> Also, I added some lines to return AVERROR(EIO) if poll() returns
> without data. This usually indicates that the device has been
> disconnected, and resulted in an endless loop.
> 
> 
> 
> Regards,
> 
> Georg

> From 8b9690660ca56fefdcbd3f4711448607536bac84 Mon Sep 17 00:00:00 2001
> From: Georg Lippitsch <georg.lippitsch at gmx.at>
> Date: Mon, 23 Apr 2012 16:01:17 +0200
> Subject: [PATCH] FireWire DV/HDV input device using libiec61883
> 
> ---
>  configure                |    3 +
>  doc/indevs.texi          |   48 ++++++
>  libavdevice/Makefile     |    1 +
>  libavdevice/alldevices.c |    1 +
>  libavdevice/iec61883.c   |  386 ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 439 insertions(+), 0 deletions(-)
>  create mode 100644 libavdevice/iec61883.c
[...]
> --- /dev/null
> +++ b/libavdevice/iec61883.c
> @@ -0,0 +1,386 @@
> +
> +/*
> + * libiec61883 interface
> + * Copyright (c) 2012 Georg Lippitsch <georg.lippitsch at gmx.at>
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <sys/poll.h>
> +#include <libraw1394/raw1394.h>
> +#include <libavc1394/avc1394.h>
> +#include <libavc1394/rom1394.h>
> +#include <libiec61883/iec61883.h>
> +#include "libavformat/dv.h"
> +#include "libavformat/mpegts.h"
> +#include "libavutil/opt.h"
> +#include "avdevice.h"
> +
> +#define MOTDCT_SPEC_ID      0x00005068

> +#define IEC61883_AUTO       0
> +#define IEC61883_DV         1
> +#define IEC61883_HDV        2

Nit: maybe an enum here (debugging), skip the comment if you don't mind

[...]
> +static int iec61883_read_packet(AVFormatContext *context, AVPacket *pkt)
> +{
> +    struct iec61883_data *dv = context->priv_data;
> +    int size;
> +    int result;
> +
> +    /**
> +     * Try to parse frames from queue. If there are none,
> +     * poll for new data from the device, and try again.
> +     */
> +
> +    while ((size = dv->parse_queue(dv, pkt)) == -1) {
> +        while ((result = poll(&dv->raw1394_poll, 1, 200)) < 0) {

> +            if (!(errno == EAGAIN || errno == EINTR)) {

Nit: De Morgan may help here, but that's pretty subjective.

[...]

> +static const AVOption options[] = {

> +    { "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" },
> +    { "hdvbuffer", "For HDV, set 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+: no capitalization

[...]

Looks good otherwise, altough I have no specific knowledge of
Firewire/libiec61883.

BTW would you mind to be enlistened as the device maintainer?
-- 
FFmpeg = Fiendish & Faithful Mystic Practical Eretic Game