diff mbox series

[FFmpeg-devel,v4,1/2] avutil/timecode: add description for SMPTE binary format

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
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Limin Wang June 30, 2020, 4:29 a.m. UTC
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(-)

Comments

Nicolas George June 30, 2020, 10:28 a.m. UTC | #1
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,
Limin Wang June 30, 2020, 2:01 p.m. UTC | #2
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
Nicolas George June 30, 2020, 2:05 p.m. UTC | #3
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 mbox series

Patch

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.