| Message ID | 1593491358-20064-1-git-send-email-lance.lmwang@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,v4,1/2] avutil/timecode: add description for SMPTE binary format | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
lance.lmwang@gmail.com (12020-06-30): > From: Limin Wang <lance.lmwang@gmail.com> > > AV_FRAME_DATA_S12M_TIMECODE is public API, so user need to know its format > by the API header instead of reading the code. > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > libavutil/frame.h | 4 ++-- > libavutil/timecode.h | 16 +++++++++++++++- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/libavutil/frame.h b/libavutil/frame.h > index 3fb8c56..c694b12 100644 > --- a/libavutil/frame.h > +++ b/libavutil/frame.h > @@ -162,8 +162,8 @@ enum AVFrameSideDataType { > /** > * Timecode which conforms to SMPTE ST 12-1. The data is an array of 4 uint32_t > * where the first uint32_t describes how many (1-3) of the other timecodes are used. > - * The timecode format is described in the av_timecode_get_smpte_from_framenum() > - * function in libavutil/timecode.c. > + * The timecode format is described in the comments of av_timecode_get_smpte_from_framenum() > + * function in libavutil/timecode.h. > */ > AV_FRAME_DATA_S12M_TIMECODE, > > diff --git a/libavutil/timecode.h b/libavutil/timecode.h > index ab38e66..b1c1887 100644 > --- a/libavutil/timecode.h > +++ b/libavutil/timecode.h > @@ -61,7 +61,21 @@ int av_timecode_adjust_ntsc_framenum2(int framenum, int fps); > * @param tc timecode data correctly initialized > * @param framenum frame number > * @return the SMPTE binary representation > - * > + * the format description as follows, note it's in system byte order: Better avoid contractions in documentation. The result is uint32_t, not char[4]: there is no byte order. > + * 31b: color frame flag (0: unsync mode, 1: sync mode) > + * 30b: drop frame flag (0: non drop, 1: drop) > + * 28b,29b: tens of frames > + * 24-27b: units of frames > + * 23b: PC (NTSC) or BGF0 (PAL) > + * 20b,22b: tens of seconds > + * 16-19b: units of seconds > + * 15b: BGF0 (NTSC) or BGF2 (PAL) > + * 12b,13b: tens of minutes > + * 8-11b: units of minutes > + * 7b: BGF2 (NTSC) or PC (PAL) > + * 6b: BGF1 > + * 4b,5b: tens of hours > + * 0b-3b: units of hours It seems more logical in the opposite order. I do not think this b suffix is very standard. Better be explicit: * bits 24-27: units of frames Also, no reason to make a special case when there are exactly two. And since several numbers are stored the same way, I would suggest to regroup: * bits 8-13: minutes, in BCD * ... * BCD numbers (6 bits): 4 lower bits for units, 2 higher bits for tens. > * @note Frame number adjustment is automatically done in case of drop timecode, > * you do NOT have to call av_timecode_adjust_ntsc_framenum2(). > * @note The frame number is relative to tc->start. Regards,
On Tue, Jun 30, 2020 at 12:28:34PM +0200, Nicolas George wrote: > lance.lmwang@gmail.com (12020-06-30): > > From: Limin Wang <lance.lmwang@gmail.com> > > > > AV_FRAME_DATA_S12M_TIMECODE is public API, so user need to know its format > > by the API header instead of reading the code. > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > > --- > > libavutil/frame.h | 4 ++-- > > libavutil/timecode.h | 16 +++++++++++++++- > > 2 files changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/libavutil/frame.h b/libavutil/frame.h > > index 3fb8c56..c694b12 100644 > > --- a/libavutil/frame.h > > +++ b/libavutil/frame.h > > @@ -162,8 +162,8 @@ enum AVFrameSideDataType { > > /** > > * Timecode which conforms to SMPTE ST 12-1. The data is an array of 4 uint32_t > > * where the first uint32_t describes how many (1-3) of the other timecodes are used. > > - * The timecode format is described in the av_timecode_get_smpte_from_framenum() > > - * function in libavutil/timecode.c. > > + * The timecode format is described in the comments of av_timecode_get_smpte_from_framenum() > > + * function in libavutil/timecode.h. > > */ > > AV_FRAME_DATA_S12M_TIMECODE, > > > > diff --git a/libavutil/timecode.h b/libavutil/timecode.h > > index ab38e66..b1c1887 100644 > > --- a/libavutil/timecode.h > > +++ b/libavutil/timecode.h > > @@ -61,7 +61,21 @@ int av_timecode_adjust_ntsc_framenum2(int framenum, int fps); > > * @param tc timecode data correctly initialized > > * @param framenum frame number > > * @return the SMPTE binary representation > > - * > > > + * the format description as follows, note it's in system byte order: > > Better avoid contractions in documentation. > > The result is uint32_t, not char[4]: there is no byte order. Sure, will remove the note. > > > + * 31b: color frame flag (0: unsync mode, 1: sync mode) > > + * 30b: drop frame flag (0: non drop, 1: drop) > > + * 28b,29b: tens of frames > > + * 24-27b: units of frames > > + * 23b: PC (NTSC) or BGF0 (PAL) > > + * 20b,22b: tens of seconds > > + * 16-19b: units of seconds > > + * 15b: BGF0 (NTSC) or BGF2 (PAL) > > + * 12b,13b: tens of minutes > > + * 8-11b: units of minutes > > + * 7b: BGF2 (NTSC) or PC (PAL) > > + * 6b: BGF1 > > + * 4b,5b: tens of hours > > + * 0b-3b: units of hours > > It seems more logical in the opposite order. > > I do not think this b suffix is very standard. Better be explicit: > > * bits 24-27: units of frames > > Also, no reason to make a special case when there are exactly two. > > And since several numbers are stored the same way, I would suggest to > regroup: > > * bits 8-13: minutes, in BCD > * ... > * BCD numbers (6 bits): 4 lower bits for units, 2 higher bits for tens. Below is my update by your suggestion, please help to review. MPTE binary representation + * the format description as follows: + * bits 0-5: hours, in BCD + * bits 6: BGF1 + * bits 7: BGF2 (NTSC) or PC (PAL) + * bits 8-13: minutes, in BCD + * bits 15: BGF0 (NTSC) or BGF2 (PAL) + * bits 16-21: seconds, in BCD + * bits 23: PC (NTSC) or BGF0 (PAL) + * bits 24-29: frames, in BCD + * bits 30: drop frame flag (0: non drop, 1: drop) + * bits 31: color frame flag (0: unsync mode, 1: sync mode) + * @note BCD numbers (6 bits): 4 lower bits for units, 2 higher bits for tens. > > * you do NOT have to call av_timecode_adjust_ntsc_framenum2(). > > * @note The frame number is relative to tc->start. > > Regards, > > -- > Nicolas George
lance.lmwang@gmail.com (12020-06-30):
> Below is my update by your suggestion, please help to review.
Fine by me. But it was only about form. I do not know timecodes at all,
so the semantic of the doc is beyond my skills.
Regards,
diff --git a/libavutil/frame.h b/libavutil/frame.h index 3fb8c56..c694b12 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -162,8 +162,8 @@ enum AVFrameSideDataType { /** * Timecode which conforms to SMPTE ST 12-1. The data is an array of 4 uint32_t * where the first uint32_t describes how many (1-3) of the other timecodes are used. - * The timecode format is described in the av_timecode_get_smpte_from_framenum() - * function in libavutil/timecode.c. + * The timecode format is described in the comments of av_timecode_get_smpte_from_framenum() + * function in libavutil/timecode.h. */ AV_FRAME_DATA_S12M_TIMECODE, diff --git a/libavutil/timecode.h b/libavutil/timecode.h index ab38e66..b1c1887 100644 --- a/libavutil/timecode.h +++ b/libavutil/timecode.h @@ -61,7 +61,21 @@ int av_timecode_adjust_ntsc_framenum2(int framenum, int fps); * @param tc timecode data correctly initialized * @param framenum frame number * @return the SMPTE binary representation - * + * the format description as follows, note it's in system byte order: + * 31b: color frame flag (0: unsync mode, 1: sync mode) + * 30b: drop frame flag (0: non drop, 1: drop) + * 28b,29b: tens of frames + * 24-27b: units of frames + * 23b: PC (NTSC) or BGF0 (PAL) + * 20b,22b: tens of seconds + * 16-19b: units of seconds + * 15b: BGF0 (NTSC) or BGF2 (PAL) + * 12b,13b: tens of minutes + * 8-11b: units of minutes + * 7b: BGF2 (NTSC) or PC (PAL) + * 6b: BGF1 + * 4b,5b: tens of hours + * 0b-3b: units of hours * @note Frame number adjustment is automatically done in case of drop timecode, * you do NOT have to call av_timecode_adjust_ntsc_framenum2(). * @note The frame number is relative to tc->start.