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

[Michael Niedermayer]

On Fri, Dec 23, 2011 at 07:28:19PM +0100, Reimar Döffinger wrote:
> On Fri, Dec 23, 2011 at 07:22:31PM +0100, Matthieu Bouron wrote:
> > 2011/12/23 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> > > On Fri, Dec 23, 2011 at 02:52:47PM +0100, Reimar Döffinger wrote:
> > >> On Fri, Dec 23, 2011 at 02:42:55PM +0100, Matthieu Bouron wrote:
> > >> > 2011/12/23 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> > >> > > On Fri, Dec 23, 2011 at 02:25:19PM +0100, Matthieu Bouron wrote:
> > >> > >> 2011/12/23 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> > >> > >> > On Fri, Dec 23, 2011 at 01:26:35AM +0100, Matthieu Bouron wrote:
> > >> > >> >> 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).
> > >> > >> >
> > >> > >> > I'd still be in favour of exporting its value as is, or would you
> > >> > >> > expect any issue with that?
> > >> > >>
> > >> > >> If i understand well, you're in favor of exporting the entire value of
> > >> > >> the timecode as an uint32_t for example and not as its string
> > >> > >> representation ? So this value should be interpreted in the
> > >> > >> dv_read_timecode function ?
> > >> > >
> > >> > > No, that would be inconsistent with what gxf does.
> > >> > > If necessary we can do that in addition.
> > >> > > No, I meant always switching between ':' and ';' depending on the
> > >> > > DF flag, even if for PAL that flag is not supposed to have a special
> > >> > > meaning.
> > >> >
> > >> > For this sample http://samples.mplayerhq.hu/DV-raw/small_test2.dv, the
> > >> > drop_frame bit is set to 1. if we do not check if it is PAL or not, it
> > >> > will display a wrong drop-frame timecode. This is why i reset the
> > >> > drop_frame value to 0 for PAL systems.
> > >>
> > >> Is that a real problem? And is that supposed to be like that or should
> > >> that sample maybe just be considered broken?
> > >> But sure, keep it if you think it is necessary but I don't like it much.
> > >
> > > Oh, but in this case maybe add that sample link to the comment, it is good to have a
> > > sample directly at hand for code that is not _obviously_ necessary.
> > 
> > I updated the comment related to the drop frame flag, to make it more clear.
> > I also changed to return value of the dv_read_timecode function.
> 
> Fine by me except for some nits not worth bother further with.
> Anyone else to review? I'm not really DV maintainer (though I doubt
> we'll hear from Roman, I don't think I've seen him in a long time).
> Michael or Baptiste, willing to have a quick look and push?
> Otherwise tell me to do it.

applied,
thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111226/111de30b/attachment.asc>