[FFmpeg-devel] NC camera patch

[Vitor Sessak]

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.

> +static int nc_read_header(AVFormatContext *s,
> +                          AVFormatParameters *ap)

Nit: you don't need a line break here.

> +{
> +    AVStream *st;
> +    st = av_new_stream(s, 0);

Nit: You can merge this as

     AVStream *st = av_new_stream(s, 0);

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

> +    /* 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);

> +    return 0;
> +}
> +
> +static int nc_read_partial_packet(AVFormatContext *s, AVPacket *pkt)

I don't think the "partial" in the name is relevant anymore

> +{
> +    uint32_t flag;
> +    int size, ret;
> +
> +    flag = get_le32(s->pb);

Nit:

     int size, ret;
     uint32_t flag = get_le32(s->pb);

[...]

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

-Vitor