[FFmpeg-devel] [PATCH] dv: add timecode to metadata

[Reimar Döffinger]

On Fri, Dec 23, 2011 at 01:07:00AM +0100, Matthieu Bouron wrote:
> +    ff          = tc_pack[1]       & 0xf;
> +    ff         += (tc_pack[1] >> 4 & 0x3) * 10;
> +    drop_frame  = tc_pack[1]  >> 6 & 0x1;
> +    ss          = tc_pack[2]       & 0xf;
> +    ss         += (tc_pack[2] >> 4 & 0x7) * 10;
> +    mm          = tc_pack[3]       & 0xf;
> +    mm         += (tc_pack[3] >> 4 & 0x7) * 10;
> +    hh          = tc_pack[4]       & 0xf;
> +    hh         += (tc_pack[4] >> 4 & 0x3) * 10;

static int bcd2int(uint8_t bcd) {
    int low  = bcd & 0xf;
    int high = bcd >> 4;
    if (low > 9 || high > 9)
        return -1;
    return low + 10*high;
}

ff = bcd2int(tc_pack[1] & 0x3f);
ss = bcd2int(tc_pack[2] & 0x7f);
mm = bcd2int(tc_pack[3] & 0x7f);
hh = bcd2int(tc_pack[4] & 0x3f);

decide on your own if you want more error handling, the -1 is obvious
enough on its own in principle.
Of course you could just skip the whole conversion to int step and do
directly
tc[0] = '0' + (tc_pack[1] >> 4);
tc[1] = '0' + (tc_pack[1] & 0xf);
tc[2] = ':'
...
which in some ways is nicer/more obvious and the result of invalid
values > 9 probably has the same 'strange' effect as it will on a lot
of hardware.
Of course whether such "unvalidated" value in timecode metadata is good
or not is up to discussion, and it's not really possible to validate
against time codes with second values like 79.
I'd still be kind of in favour since it most directly exports the data
that is actually in the file, but some people dislike that.


> +    ret = avio_read(s->pb, partial_frame, partial_frame_size);
> +    if (ret < partial_frame_size)
> +        goto finish;
[...]
> +finish:
> +    av_free(partial_frame);
> +    avio_seek(s->pb, pos, SEEK_SET);
> +    return ret;

That is not quite right I think, you should not return >= 0 on error.
So for avio_read returning < 0 you should return that value,
however for avio_read returning >= 0 and < partial_frame_size
you should return -1 or AVERROR_EOF or AVERROR(EIO) or such.