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

[Matthieu Bouron]

2011/12/22 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> 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?

The format is described in the SMPTE 314M.
For 525/60 Systems:

CF (1b)  DF(1b)         TENS OF FRAME(2b)    UNITS OF FRAME(4b)
PC (1b)               TENS OF SECONDS (3b)    UNITS OF SECONDS(4b)
BGF0(1b)              TENS OF MINUTES (3b)    UNITS OF MINUTES(4b)
BGF2(1b) BGF1(1b)   TENS OF HOURS(2b)    UNITS OF HOURS (4b)

The only difference for PAL system, is that the DF (drop_frame) flag
is ignored (defined as arbitrary).

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

I don't get it, can you explain me your idea ?

>> +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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel