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

[Reimar Döffinger]

On Thu, Dec 22, 2011 at 10:29:52PM +0100, Matthieu Bouron wrote:
> +static int dv_extract_timecode(DVDemuxContext* c, uint8_t* frame, char tc[32])
> +{
> +    int hh, mm, ss, ff, df;
> +    const uint8_t* tc_pack;

Having the space between type and * and not between * and name is more
consistent with the rest of the code (and also more consistent with the
actual semantics).

> +    ff  = tc_pack[1]         & 0xf;
> +    ff += ((tc_pack[1] >> 4) & 0x3) * 10;
> +    df  = ((tc_pack[1] >> 6) & 0x1);
> +    df  = (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) & 0x1) * 10;

What is the exact format?
This looks like BCD, however hh == 23 would have to be
represented as 0x1d, which is definitely not BCD.
Also you assign the same value twice to "df" (whatever that may stand
for - looking at the later comment "drop frame", but a bit longer
variable names might be good).
If it is BCD, you should probably add a bcd2decimal function and make
it complain about invalid values.

> +    // DV PAL is non-dropframe
> +    if(c->sys->ltc_divisor == 25 || c->sys->ltc_divisor == 50) {
> +        df = 0;
> +    }

IMO this lacks an argument why we should on-purpose mangle the time-code
data. If the file is broken silently fixing it up is questionable
(encourages bad implementations) and can be a major issue to debug if
ever our "mangling" goes wrong.

> +static int dv_read_timecode(AVFormatContext *s) {
> +    char timecode[32];
> +    int64_t pos = avio_tell(s->pb);
> +
> +    // Read 3 DIFs: Header DIF and 2 Subcode DIFs.
> +    int partial_frame_size = 3 * 80;
> +    uint8_t *partial_frame = av_mallocz(sizeof(uint8_t) * partial_frame_size);

sizeof(uint8_t) is sure to be 1 for anything supported.
But even if not, sizeof(*partial_frame) is a more feature-proof way
of getting the value.

> +    RawDVContext *c = s->priv_data;
> +    if (avio_read(s->pb, partial_frame, partial_frame_size) <= 0) {
> +        av_free(partial_frame);
> +        avio_seek(s->pb, pos, SEEK_SET);
> +        return AVERROR(EIO);

If the result is < 0 you should return that.
Also you don't handle the case of the result being in-between 0 and
partial_frame_size (i.e. a partial read).

> +    av_free(partial_frame);
> +    avio_seek(s->pb, pos, SEEK_SET);

Also a goto is more maintainable than duplicating the code usually.

> +    if (s->pb->seekable) {
> +        dv_read_timecode(s);
> +    }

Nit: We usually don't put one-line code in {}, though I am not sure if
there is so much of a consensus any more on that.