[FFmpeg-devel] [PATCH] FireWire DV/HDV input device using libiec61883
Georg Lippitsch
georg.lippitsch at gmx.at
Wed Apr 25 21:53:00 CEST 2012
Am 24.04.2012, 15:50 Uhr, schrieb Stefano Sabatini <stefasab at gmail.com>:
> Would you mind adding some generic information in doc/indevs.texi
> about the device? Anything which is required to know in order to use
> the device and one or more examples may fit well.
Ok, will submit in the next patch.
> Nit+: inverted order (from system, to libav* library to current dir is
> preferred)
Fixed
>> +#define MOTDCT_SPEC_ID 0x00005068
>
>> +#define IEC61883_AUTO 0
>> +#define IEC61883_DV 1
>> +#define IEC61883_HDV 2
>
> Nit+: this could be an enum
They are used by AVOption, and there always defines are used IIUC. Or am I
wrong here?
>> + * For DV, one packet correspondets exactly to one frame.
>
> corresponds
Fixed
> nit+: here and below, non complete sentences should not be
> Capitalized, like in:
>
> This is a complete sentence.
> an incomplete sentence
Fixed
> typo: quueue
Fixed
>> + int isHDV; ///< Before connecting, find
>> out if DV/HDV
>
> Nit: camelStyle is avoided in libav*, snake_style is preferred
Fixed
> Also if I understand it correctly this is not a binary field, but a
> type (auto/DV/HDV) (may slightly help understandibility)
It is actually used twice: Once by AVOption, which sets it to either
auto/DV/HDV.
After detection in read_header, it is used as binary.
This might be confusing, but I didn't want to introduce an extra variable.
Or would you prefer that?
>> +static int iec61883_callback_dv(unsigned char *data, int length,
>> + int complete, void *callback_data)
>
> weird indent
Fixed
>> + packet->buf = av_malloc(length);
>
> missing NULL check?
All NULL checks fixed.
>> + while(dv->queue_first) {
>
> nit++: while_(
Fixed
>> + /* Select first AV/C tape reccorder player node */
>
> typo: reccorder
Fixed
>> + while ((size = dv->parse_queue(dv, pkt)) == -1) {
>> + while ((result = poll(&dv->raw1394_poll, 1, 200)) < 0) {
>> + if (!(errno == EAGAIN || errno == EINTR)) {
>
>> + av_log(context, AV_LOG_ERROR, "Raw1394 poll.\n");
>
> "Raw1394 poll error occurred.\n"
Fixed
>
> Or you could store the error and print the corresponding description
> to the output, I mean:
> ret = ...
> if ((!(ret == ...)
> av_log(context, ERROR, "Raw1394 poll error occurred: %s\n",
> strerror_r(errbuf, errbuf_size, err));
>
> but maybe overkill
I think it's overkill, particularly because I've never seen this actually
happen.
>
>> + { "auto", "", 0, AV_OPT_TYPE_CONST, {.dbl = IEC61883_AUTO}, 0,
>> 0, AV_OPT_FLAG_DECODING_PARAM, "dvtype" },
>> + { "dv", "", 0, AV_OPT_TYPE_CONST, {.dbl = IEC61883_DV}, 0,
>> 0, AV_OPT_FLAG_DECODING_PARAM, "dvtype" },
>> + { "hdv" , "", 0, AV_OPT_TYPE_CONST, {.dbl = IEC61883_HDV}, 0,
>> 0, AV_OPT_FLAG_DECODING_PARAM, "dvtype" },
>
> please add a short description
Fixed
>
>> + { "hdvbuffer", "For HDV, 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: "for HDV, set buffer size ..."
Fixed
>> +AVInputFormat ff_iec61883_demuxer = {
>> + .name = "iec61883",
>> + .long_name = NULL_IF_CONFIG_SMALL("libiec61883 (new DV1394)
>> A/V grab"),
>
> Nit, subjective: A/V grab => A/V input device
Fixed
Regards,
Georg
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Fix-typos-and-some-nits-in-libiec61883-input-device.patch
Type: text/x-patch
Size: 11038 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120425/a2431cdd/attachment.bin>
More information about the ffmpeg-devel
mailing list