[FFmpeg-devel] NC camera patch

[nicolas martin]

> 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.

Nicolas
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch.txt
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090109/7e910f9d/attachment.txt>
-------------- next part --------------

>
>
> -Vitor
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel