[FFmpeg-devel] NC camera patch

Michael Niedermayer michaelni
Sat Jan 10 14:49:35 CET 2009


On Fri, Jan 09, 2009 at 10:39:19PM -0500, nicolas martin wrote:
>
>> nicolas martin wrote:
>>>
>>>> nicolas martin wrote:
>>>>
>>>>> Hi all,
>>>>>
>>>>> I finally made some code to read the camera feed sent by NC46** types
>>>>> cameras.
>>>>>
>>>>> Thanks for reading and sharing your thoughts !
>>>>
>>>> The format looks quite simple and your code quite complicated.
>>>> Maybe I missed some subtlety in the format.
>>>> So let me first describe the format as I understand it:
>>>> Each frame is composed of:
>>>> - A header (16 bytes)
>>>>   * 4 bytes : a flag (eg: 0x1A5  (big-endian))
>>>>   * 1 byte  : unknown/unused
>>>>   * 2 bytes : data_size (only when flag == 0x1A5)
>>>>   * 9 bytes : unknown/unused
>>>> - MPEG4 data frame (data_size bytes)
>>>> Please correct me if there's something wrong here.
>>>>
>>>> If my description is right, you could write your frame reader
>>>> function trivially, with something like that:
>>>>
>>>> flag = get_be32(pb);
>>>>        get_byte(pb);
>>>> size = get_le16(pb);
>>>> url_fskip(pb, 9);
>>>> if (flag != 0x1A5)
>>>>     /* error handling */;
>>>> av_new_packet(pkt, size)
>>>> get_buffer(pb, pkt->data, size);
>>>>
>>>> You need some more error checking, etc...
>>>> But basically, this code should be enough.
>>>>
>>>> BTW: you should upload a sample file like described in [1] to allow
>>>> other developers to test your code.
>>>>
>>> Ok I uploaded a sample in nc_camera
>>>
>>>> Oh, and your nc_read_header() seems to contain code which is
>>>> totally unrelated to your format (MJPEG, DIRAC, etc...).
>>>> And you don't need to use s->iformat->value if it's hardcoded
>>>> to MPEG4.
>>>
>>> I joined the version I modified using your tips.
>>> I still don't know what to put in nc_read_header() by the way
>>
>> Just my two cents...
>>
>> [...]
>>
>>> +/*
>>> + * NC cameras feed demuxer.
>>> + * Copyright (c) 2008  Nicolas Martin <martinic at iro.umontreal.ca>, 
>>> Edouard Auvinet
>>> + *
>>> + * 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 "avformat.h"
>>> +
>>> +#define NC_VIDEO_FLAG 0xA5010000
>>> +
>>> +static int nc_probe(AVProbeData *probe_packet)
>>> +{
>>> +    return 
>>> (AV_RL32(probe_packet->buf)==NC_VIDEO_FLAG?AVPROBE_SCORE_MAX:0);
>>> +}
>>> +
>>
>> If I understood well the format, the next four bytes should also be
>> NC_VIDEO_FLAG for valid files. So, its better to check also for them to
>> avoid false positives.
>
> Just the first four bytes should be NC_VIDEO_FLAG, the next one is unused 
> and the two following are the size of the next paquet.
>
>>
>>
>>> +static int nc_read_header(AVFormatContext *s,
>>> +                          AVFormatParameters *ap)
>>
>> Nit: you don't need a line break here.
>
> Changed.
>
>>
>>
>>> +{
>>> +    AVStream *st;
>>> +    st = av_new_stream(s, 0);
>>
>> Nit: You can merge this as
>>
>>     AVStream *st = av_new_stream(s, 0);
>
> Changed.
>
>>
>>
>>> +    if (!st)
>>> +        return AVERROR(ENOMEM);
>>> +
>>> +    st->codec->codec_type = CODEC_TYPE_VIDEO;
>>
>>> +    st->codec->codec_id = s->iformat->value;
>>
>> Here you can do as Aurelien sugested and set directly codec_id to
>> CODEC_ID_MPEG4...
>
> Changed.
>
>>
>>
>>> +    /* for MJPEG, specify frame rate */
>>> +    /* for MPEG-4 specify it, too (most MPEG-4 streams do not have the 
>>> fixed_vop_rate set ...)*/
>>> +    if (ap->time_base.num) {
>>> +        av_set_pts_info(st, 64, ap->time_base.num, ap->time_base.den);
>>> +    } else if ( st->codec->codec_id == CODEC_ID_MJPEG ||
>>> +                st->codec->codec_id == CODEC_ID_MPEG4 ||
>>> +                st->codec->codec_id == CODEC_ID_DIRAC ||
>>> +                st->codec->codec_id == CODEC_ID_H264) {
>>> +        av_set_pts_info(st, 64, 1, 25);
>>> +    }
>>
>> ...and them this block is the same as just
>>
>>     av_set_pts_info(st, 64, 1, 25);
>
> Changed.
>
>>
>>
>>> +    return 0;
>>> +}
>>> +
>>> +static int nc_read_partial_packet(AVFormatContext *s, AVPacket *pkt)
>>
>> I don't think the "partial" in the name is relevant anymore
>
> Changed.
>
>>
>>
>>> +{
>>> +    uint32_t flag;
>>> +    int size, ret;
>>> +
>>> +    flag = get_le32(s->pb);
>>
>> Nit:
>>
>>     int size, ret;
>>     uint32_t flag = get_le32(s->pb);
>>
>> [...]
>
> Changed.
>
>>
>>
>>> +AVInputFormat nc_demuxer = {
>>> +    "nc",
>>> +    NULL_IF_CONFIG_SMALL("NC camera feed format"),
>>> +    NULL,
>>> +    nc_probe,
>>> +    nc_read_header,
>>> +    nc_read_partial_packet,
>>> +    .extensions = "v",
>>> +    .value = CODEC_ID_MPEG4,
>>
>> As Aurelien said, if you set codec_id directly to CODEC_ID_MPEG4 in
>> nc_read_header(), this last line is unnecessary.
>
> Changed.
>
> The new patch is attached.

[...]
> +static int nc_read_header(AVFormatContext *s, AVFormatParameters *ap)
> +{
> +    AVStream *st = av_new_stream(s, 0);
> +    if (!st)
> +        return AVERROR(ENOMEM);
> +
> +    st->codec->codec_type = CODEC_TYPE_VIDEO;
> +    st->codec->codec_id = CODEC_ID_MPEG4;

> +    st->need_parsing = AVSTREAM_PARSE_FULL;

Does the format need this? if not it should be removed otherwise it
can stay


[...]
> +static int nc_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +    int size, ret;
> +    uint32_t flag = get_le32(s->pb);

> +    if (flag != NC_VIDEO_FLAG) {
> +        av_log(NULL, AV_LOG_DEBUG, "wrong flag for nc video format : %u\n", flag);
> +        return AVERROR_INVALIDDATA;
> +    }

this is unneeded, it already was checked in probe


[...]
> +    if (av_new_packet(pkt, size) < 0)
> +        return AVERROR(EIO);
> +
> +    pkt->pos = url_ftell(s->pb);
> +    pkt->stream_index = 0;
> +
> +    ret = get_buffer(s->pb, pkt->data, size);
> +    if (ret<=0) {
> +        av_free_packet(pkt);
> +        return AVERROR(EIO);
> +    }

this can be simplified with av_get_packet()


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090110/c56f5513/attachment.pgp>



More information about the ffmpeg-devel mailing list