[FFmpeg-devel] [PATCH V2] avformat: Add Dynacolor MVC Demuxer

[Moritz Barsnick]

On Sat, Mar 28, 2020 at 13:32:42 +0000, Tom Needham wrote:
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA

You've inlined your patch again, and again it's corrupted by newlines.

> +unsigned char ff_dyna_callback_checksum(tBasicIdx *header)
> +{
> +    int i;
> +    unsigned char chksum = 0, *pHeader;
> +    pHeader = (unsigned char *)header;
> +    if (*pHeader == '@' && *(pHeader + 1) == '2') {

Is *header always guaranteed a size of at least two?

> +        for (i = 0; i < sizeof(tBasicIdx) - 1; ++i) {

ffmpeg prefers "i++", and sizeof(variable) over sizeof(type).

> void ff_dyna_make_drv_header(tHeader *pHdr, int serno, unsigned int size, int pic_type)

Is this used anywhere? From its contents, it looks like a function for
a muxer, not a demuxer:

> +    // set basic header
> +    pHdr->Basic.Header1 = '@';
> +    pHdr->Basic.Header2 = '2';
> +    pHdr->Basic.QI = (pHdr->Basic.WidthBase) ? 1 : 2;
[...]

> +tHeader *ff_dyna_get_audio_header(tHeader *frame, unsigned int chksum, unsigned int audioserno)
> +{
> +    //audio is ms adpcm. Each package size is 32(tHeader) +2048(ms adpcm).
> +    tHeader *Header = frame;
> +    Header->Basic.Header1 = '@';
> +    Header->Basic.Header2 = '2';
> +    Header->Basic.Size = 2048;

Ditto.

> +    av_log(ctx, AV_LOG_DEBUG, "Parsing tBasicIdx Header \n");

You do have a log of debug messages (and a lot of empty spaces at the
end of each). They should probably not go into the final version.

> +    cdata_L->Basic.NTSC_PAL = *(basicIdx_L)&0x01;
> +    cdata_L->Basic.ImgMode = *(basicIdx_L) >> 1 & 0x01;
> +    cdata_L->Basic.Audio = *(basicIdx_L) >> 2 & 0x01;
> +    cdata_L->Basic.DST = *(basicIdx_L) >> 3 & 0x01;
> +    cdata_L->Basic.Covert = *(basicIdx_L) >> 4 & 0x01;
> +    cdata_L->Basic.Vloss = *(basicIdx_L) >> 5 & 0x01;
> +    cdata_L->Basic.AlarmIn = *(basicIdx_L) >> 6 & 0x01;
> +    cdata_L->Basic.Motion = *(basicIdx_L) >> 7 & 0x01;
> +    cdata_L->Basic.ExtraEnable = *(basicIdx_L) >> 8 & 0x01;
> +    cdata_L->Basic.EventEnable = *(basicIdx_L) >> 9 & 0x01;
> +    cdata_L->Basic.PPS = *(basicIdx_L) >> 10 & 0x40;
> +    cdata_L->Basic.Type = *(basicIdx_L) >> 16 & 0x08;
> +    cdata_L->Basic.Channel = *(basicIdx_L) >> 19 & 0x20;
> +    cdata_L->Basic.Chksum = *(basicIdx_L) >> 24 & 0xFF;

For readability, you could align all the '=', making it easier to see
which bits get mapped where.

(Other places as well.)

> +        if (!stream_format) {
> +            av_log(ctx, AV_LOG_WARNING, "File Has Unkown Stream Format:
> 0x%02X", stream_format);
> +            return AVERROR_PATCHWELCOME;

Please use avpriv_report_missing_feature().

> +                if (!ret) {
> +                    return AVERROR_PATCHWELCOME;

Also here.

> +    hdr->unused_0 = (unsigned int)(pes[4] << 24 | pes[5] << 16 | pes[6] <<
> 8 | pes[7]);
> +    hdr->unused_1 = (unsigned int)(pes[8] << 24 | pes[9] << 16 | pes[10]
> << 8 | pes[11]);
> +    hdr->unused_2 = (unsigned int)(pes[12] << 24 | pes[13] << 16 | pes[14]
> << 8 | pes[15]);
> +    hdr->unused_3 = (unsigned int)(pes[16] << 24 | pes[17] << 16 | pes[18]
> << 8 | pes[19]);
> +    hdr->unused_4 = (unsigned int)(pes[20] << 24 | pes[21] << 16 | pes[22]
> << 8 | pes[23]);

There might be macros in ffmpeg for this.

> +static int dyna_read_probe(const AVProbeData *p)
> +{
> +    unsigned char* bytes = av_malloc(p->buf_size);
> +
> +    for (int i = 0; i < p->buf_size; i++) {
> +        bytes[i] = p->buf[i];
> +    }
Is this basically a memcpy?

> +    if (bytes[0] == 0x40 && bytes[1] == 0x32 && bytes[2] == 0x10 &&
> bytes[3] == 0x20)

OK, but there's also a macro for this in ffmpeg.

> +    tPesHeader *pes;
[...]
> +    start = av_malloc(sizeof(AVIOContext));
> +    memcpy(start, pb, sizeof(AVIOContext));
> +
> +    pesData = av_malloc_array(PENTAMICRO_PES_HEADER_SIZE, 1);
> +    pes = av_malloc(sizeof(tPesHeader));
[...]
> +    ret = ff_dyna_read_file_header(ctx, pb, pesData, pes, &size, &time,
> +                            &priv->header, &basicIdx_H, &basicIdx_L);
> +
> +    if (ret)
> +        return AVERROR_INVALIDDATA;

You do realize that every malloc() has to be complemented with a
free(), even in the error case? See other demuxers: You'd have a goto
here, and a cleanup block at the end of the function, with a label.

> +    size = (priv->pes_header.size_bit7to0 & 0xFF)
> +    | ((priv->pes_header.size_bit10to8 & 0x04)) << 15
> +    | ((priv->pes_header.size_bit14to11 & 0x08) << 11)
> +    | ((priv->pes_header.size_bit21to15 & 0x7F) << 8);

Wrong indentation.

> +    if (ret < 0)
> +        return ret;
> +
> +    return ret;

I'm not sure this makes sense. ;-)

> +static int dyna_read_close(AVFormatContext *ctx)
> +{
> +    return 0;
> +}

You can probably just not define a close callback if it doesn't do
anything (but I'm not sure).

> +    size = (priv->pes_header.size_bit7to0 & 0xFF)
> +    | ((priv->pes_header.size_bit10to8 & 0x04)) << 15
> +    | ((priv->pes_header.size_bit14to11 & 0x08) << 11)
> +    | ((priv->pes_header.size_bit21to15 & 0x7F) << 8);

Wrong indentation.

> +    if (avio_seek(ctx->pb, size, SEEK_SET) < 0)
> +        return -1;

I'm not sure: Should you be returning an AVERROR here? (Too lazy to
check, sorry.)

> +} tBasicIdx;

ffmpeg doesn't name its structs/types this way.

Cheers,
Moritz