diff mbox series

[FFmpeg-devel,3/4] avutil/timecode: add function av_timecode_get_smpte_by_tc_string()

Message ID 1592577334-19049-3-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/4] avfilter/vf_drawtext: add option to draw timecode in S12M side data | expand

Checks

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

Commit Message

Lance Wang June 19, 2020, 2:35 p.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavutil/timecode.c | 26 ++++++++++++++++++++++++++
 libavutil/timecode.h |  9 +++++++++
 2 files changed, 35 insertions(+)

Comments

Nicolas George June 19, 2020, 2:48 p.m. UTC | #1 
lance.lmwang@gmail.com (12020-06-19):
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavutil/timecode.c | 26 ++++++++++++++++++++++++++
>  libavutil/timecode.h |  9 +++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/libavutil/timecode.c b/libavutil/timecode.c
> index 60077ba..7a7ed1d 100644
> --- a/libavutil/timecode.c
> +++ b/libavutil/timecode.c
> @@ -81,6 +81,32 @@ uint32_t av_timecode_get_smpte_from_framenum(const AVTimecode *tc, int framenum)
>             (hh % 10);        // units of hours
>  }
>  
> +uint32_t av_timecode_get_smpte_by_tc_string(const char *buf)
> +{
> +    char c;
> +    int hh, mm, ss, ff, drop;
> +

> +    if (sscanf(buf, "%d:%d:%d%c%d", &hh, &mm, &ss, &c, &ff) != 5) {

This accepts garbage at the end. Is it on purpose?

> +        return AVERROR_INVALIDDATA;

You cannot return an error code with that function signature.

> +    }
> +    drop = c != ':' ? AV_TIMECODE_FLAG_DROPFRAME : 0; // drop if ';', '.', ...
> +
> +    return 0         << 31 | // color frame flag (0: unsync mode, 1: sync mode)
> +           drop      << 30 | // drop  frame flag (0: non drop,    1: drop)
> +           (ff / 10) << 28 | // tens  of frames
> +           (ff % 10) << 24 | // units of frames
> +           0         << 23 | // PC (NTSC) or BGF0 (PAL)
> +           (ss / 10) << 20 | // tens  of seconds
> +           (ss % 10) << 16 | // units of seconds
> +           0         << 15 | // BGF0 (NTSC) or BGF2 (PAL)
> +           (mm / 10) << 12 | // tens  of minutes
> +           (mm % 10) <<  8 | // units of minutes
> +           0         <<  7 | // BGF2 (NTSC) or PC (PAL)
> +           0         <<  6 | // BGF1
> +           (hh / 10) <<  4 | // tens  of hours
> +           (hh % 10);        // units of hours
> +}
> +
>  char *av_timecode_make_string(const AVTimecode *tc, char *buf, int framenum)
>  {
>      int fps = tc->fps;
> diff --git a/libavutil/timecode.h b/libavutil/timecode.h
> index 37c1361..7bb4b78 100644
> --- a/libavutil/timecode.h
> +++ b/libavutil/timecode.h
> @@ -71,6 +71,15 @@ int av_timecode_adjust_ntsc_framenum2(int framenum, int fps);
>  uint32_t av_timecode_get_smpte_from_framenum(const AVTimecode *tc, int framenum);
>  
>  /**
> + * Convert TC string to SMPTE 12M binary representation.
> + *
> + * @param buf      TC string
> + * @return         the SMPTE binary representation, or AVERROR otherwise
> + *
> + */
> +uint32_t av_timecode_get_smpte_by_tc_string(const char *buf);
> +
> +/**
>   * Load timecode string in buf.
>   *
>   * @param buf      destination buffer, must be at least AV_TIMECODE_STR_SIZE long

Regards,
Lance Wang June 19, 2020, 3:09 p.m. UTC | #2 
On Fri, Jun 19, 2020 at 04:48:46PM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-06-19):
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavutil/timecode.c | 26 ++++++++++++++++++++++++++
> >  libavutil/timecode.h |  9 +++++++++
> >  2 files changed, 35 insertions(+)
> > 
> > diff --git a/libavutil/timecode.c b/libavutil/timecode.c
> > index 60077ba..7a7ed1d 100644
> > --- a/libavutil/timecode.c
> > +++ b/libavutil/timecode.c
> > @@ -81,6 +81,32 @@ uint32_t av_timecode_get_smpte_from_framenum(const AVTimecode *tc, int framenum)
> >             (hh % 10);        // units of hours
> >  }
> >  
> > +uint32_t av_timecode_get_smpte_by_tc_string(const char *buf)
> > +{
> > +    char c;
> > +    int hh, mm, ss, ff, drop;
> > +
> 
> > +    if (sscanf(buf, "%d:%d:%d%c%d", &hh, &mm, &ss, &c, &ff) != 5) {
> 
> This accepts garbage at the end. Is it on purpose?

No, in fact I reuse the code from av_timecode_init_from_string(), any suggestion with safety?


> 
> > +        return AVERROR_INVALIDDATA;
> 
> You cannot return an error code with that function signature.

My fault, I'll add a parameter to get the binary TC.

> 
> > +    }
> > +    drop = c != ':' ? AV_TIMECODE_FLAG_DROPFRAME : 0; // drop if ';', '.', ...
> > +
> > +    return 0         << 31 | // color frame flag (0: unsync mode, 1: sync mode)
> > +           drop      << 30 | // drop  frame flag (0: non drop,    1: drop)
> > +           (ff / 10) << 28 | // tens  of frames
> > +           (ff % 10) << 24 | // units of frames
> > +           0         << 23 | // PC (NTSC) or BGF0 (PAL)
> > +           (ss / 10) << 20 | // tens  of seconds
> > +           (ss % 10) << 16 | // units of seconds
> > +           0         << 15 | // BGF0 (NTSC) or BGF2 (PAL)
> > +           (mm / 10) << 12 | // tens  of minutes
> > +           (mm % 10) <<  8 | // units of minutes
> > +           0         <<  7 | // BGF2 (NTSC) or PC (PAL)
> > +           0         <<  6 | // BGF1
> > +           (hh / 10) <<  4 | // tens  of hours
> > +           (hh % 10);        // units of hours
> > +}
> > +
> >  char *av_timecode_make_string(const AVTimecode *tc, char *buf, int framenum)
> >  {
> >      int fps = tc->fps;
> > diff --git a/libavutil/timecode.h b/libavutil/timecode.h
> > index 37c1361..7bb4b78 100644
> > --- a/libavutil/timecode.h
> > +++ b/libavutil/timecode.h
> > @@ -71,6 +71,15 @@ int av_timecode_adjust_ntsc_framenum2(int framenum, int fps);
> >  uint32_t av_timecode_get_smpte_from_framenum(const AVTimecode *tc, int framenum);
> >  
> >  /**
> > + * Convert TC string to SMPTE 12M binary representation.
> > + *
> > + * @param buf      TC string
> > + * @return         the SMPTE binary representation, or AVERROR otherwise
> > + *
> > + */
> > +uint32_t av_timecode_get_smpte_by_tc_string(const char *buf);
> > +
> > +/**
> >   * Load timecode string in buf.
> >   *
> >   * @param buf      destination buffer, must be at least AV_TIMECODE_STR_SIZE long
> 
> Regards,
> 
> -- 
>   Nicolas George
Nicolas George June 19, 2020, 3:15 p.m. UTC | #3 
lance.lmwang@gmail.com (12020-06-19):
> No, in fact I reuse the code from av_timecode_init_from_string(), any suggestion with safety?

For starters: DO NOT REUSE CODE.

Use it. Call av_timecode_init_from_string().

To test the garbage at the end of the string: ask scanf() where scanning
stopped, check it is the end of the string.

Regards,
Lance Wang June 20, 2020, 2:08 a.m. UTC | #4 
On Fri, Jun 19, 2020 at 05:15:10PM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-06-19):
> > No, in fact I reuse the code from av_timecode_init_from_string(), any suggestion with safety?
> 
> For starters: DO NOT REUSE CODE.
> 
> Use it. Call av_timecode_init_from_string().

No, different function, if I can use it, then I'll use it.

> 
> To test the garbage at the end of the string: ask scanf() where scanning
> stopped, check it is the end of the string.

do you have a broken string for my condition, I'm glad to test and fix it?

> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George June 20, 2020, 12:28 p.m. UTC | #5 
lance.lmwang@gmail.com (12020-06-20):
> No, different function, if I can use it, then I'll use it.

Duplicating parsing code is a big no.

> do you have a broken string for my condition, I'm glad to test and fix it?

Any valid string with anything appended at the end.

Regards,
Lance Wang June 20, 2020, 12:48 p.m. UTC | #6 
On Sat, Jun 20, 2020 at 02:28:39PM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-06-20):
> > No, different function, if I can use it, then I'll use it.
> 
> Duplicating parsing code is a big no.
av_timecode_init_from_string() will initialize the AVTimecode structure,
it'll do more functions, if don't duplicate the code. I can do like below:
1. use av_timecode_init_from_string() to init the tc
2. use av_timecode_get_smpte_from_framenum() to get the smpte binary

do you think it's OK?


> 
> > do you have a broken string for my condition, I'm glad to test and fix it?
> 
> Any valid string with anything appended at the end.

OK, I'll try to test it.

> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George June 20, 2020, 12:51 p.m. UTC | #7 
lance.lmwang@gmail.com (12020-06-20):
> av_timecode_init_from_string() will initialize the AVTimecode structure,
> it'll do more functions, if don't duplicate the code. I can do like below:
> 1. use av_timecode_init_from_string() to init the tc
> 2. use av_timecode_get_smpte_from_framenum() to get the smpte binary
> 
> do you think it's OK?

Yes, I think it is the correct way of doing it: using existing functions
instead of copying their code.

Regards,
Lance Wang June 20, 2020, 2:27 p.m. UTC | #8 
On Sat, Jun 20, 2020 at 02:51:59PM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-06-20):
> > av_timecode_init_from_string() will initialize the AVTimecode structure,
> > it'll do more functions, if don't duplicate the code. I can do like below:
> > 1. use av_timecode_init_from_string() to init the tc
> > 2. use av_timecode_get_smpte_from_framenum() to get the smpte binary
> > 
> > do you think it's OK?
> 
> Yes, I think it is the correct way of doing it: using existing functions
> instead of copying their code.

Thanks, have updated with 2 patch only.

> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavutil/timecode.c b/libavutil/timecode.c
index 60077ba..7a7ed1d 100644
--- a/libavutil/timecode.c
+++ b/libavutil/timecode.c
@@ -81,6 +81,32 @@  uint32_t av_timecode_get_smpte_from_framenum(const AVTimecode *tc, int framenum)
            (hh % 10);        // units of hours
 }
 
+uint32_t av_timecode_get_smpte_by_tc_string(const char *buf)
+{
+    char c;
+    int hh, mm, ss, ff, drop;
+
+    if (sscanf(buf, "%d:%d:%d%c%d", &hh, &mm, &ss, &c, &ff) != 5) {
+        return AVERROR_INVALIDDATA;
+    }
+    drop = c != ':' ? AV_TIMECODE_FLAG_DROPFRAME : 0; // drop if ';', '.', ...
+
+    return 0         << 31 | // color frame flag (0: unsync mode, 1: sync mode)
+           drop      << 30 | // drop  frame flag (0: non drop,    1: drop)
+           (ff / 10) << 28 | // tens  of frames
+           (ff % 10) << 24 | // units of frames
+           0         << 23 | // PC (NTSC) or BGF0 (PAL)
+           (ss / 10) << 20 | // tens  of seconds
+           (ss % 10) << 16 | // units of seconds
+           0         << 15 | // BGF0 (NTSC) or BGF2 (PAL)
+           (mm / 10) << 12 | // tens  of minutes
+           (mm % 10) <<  8 | // units of minutes
+           0         <<  7 | // BGF2 (NTSC) or PC (PAL)
+           0         <<  6 | // BGF1
+           (hh / 10) <<  4 | // tens  of hours
+           (hh % 10);        // units of hours
+}
+
 char *av_timecode_make_string(const AVTimecode *tc, char *buf, int framenum)
 {
     int fps = tc->fps;
diff --git a/libavutil/timecode.h b/libavutil/timecode.h
index 37c1361..7bb4b78 100644
--- a/libavutil/timecode.h
+++ b/libavutil/timecode.h
@@ -71,6 +71,15 @@  int av_timecode_adjust_ntsc_framenum2(int framenum, int fps);
 uint32_t av_timecode_get_smpte_from_framenum(const AVTimecode *tc, int framenum);
 
 /**
+ * Convert TC string to SMPTE 12M binary representation.
+ *
+ * @param buf      TC string
+ * @return         the SMPTE binary representation, or AVERROR otherwise
+ *
+ */
+uint32_t av_timecode_get_smpte_by_tc_string(const char *buf);
+
+/**
  * Load timecode string in buf.
  *
  * @param buf      destination buffer, must be at least AV_TIMECODE_STR_SIZE long