[FFmpeg-devel] [PATCH] dv: add timecode to metadata
Reimar Döffinger
Reimar.Doeffinger at gmx.de
Thu Dec 22 23:54:30 CET 2011
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.
More information about the ffmpeg-devel
mailing list