diff mbox series

[FFmpeg-devel,v2,2/4] avcodec/hevcdec: create AVFrame side data from HEVC timecodes like H.264

Message ID 1592410030-6553-2-git-send-email-lance.lmwang@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,v2,1/4] avcodec/hevc_sei: support HEVC timecode decode | 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 17, 2020, 4:07 p.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavcodec/hevcdec.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

Comments

Josh Dekker June 23, 2020, 9:18 p.m. UTC | #1 
On 17/06/2020 17:07, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>   libavcodec/hevcdec.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 44 insertions(+)
> 
> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> index c9e28f5..39abbb9 100644
> --- a/libavcodec/hevcdec.c
> +++ b/libavcodec/hevcdec.c
> @@ -2808,6 +2808,50 @@ static int set_side_data(HEVCContext *s)
>       }
>       s->sei.unregistered.nb_buf_ref = 0;
>   
> +    if (s->sei.timecode.present) {
> +        uint32_t tc = 0;
> +        uint32_t *tc_sd;
> +        AVFrameSideData *tcside = av_frame_new_side_data(out, AV_FRAME_DATA_S12M_TIMECODE,
> +                                                         sizeof(uint32_t) * 4);
> +        if (!tcside)
> +            return AVERROR(ENOMEM);
> +
> +        tc_sd = (uint32_t*)tcside->data;
> +        tc_sd[0] = s->sei.timecode.num_clock_ts;
> +
> +        for (int i = 0; i < tc_sd[0]; i++) {
> +            uint32_t frames;
> +
> +            /* For SMPTE 12-M timecodes, frame count is a special case if > 30 FPS.
> +               See SMPTE ST 12-1:2014 Sec 12.1 for more info. */
> +            if (av_cmp_q(s->avctx->framerate, (AVRational) {30, 1}) == 1) {
> +                frames = s->sei.timecode.n_frames[i] / 2;
> +                if (s->sei.timecode.n_frames[i] % 2 == 1) {
> +                    if (av_cmp_q(s->avctx->framerate, (AVRational) {50, 1}) == 0)
> +                        tc |= (1 << 7);
> +                    else
> +                        tc |= (1 << 23);
> +                }
> +            } else {
> +                frames = s->sei.timecode.n_frames[i];
> +            }
> +
> +            tc |= s->sei.timecode.cnt_dropped_flag[i] << 30;
> +            tc |= (frames / 10) << 28;
> +            tc |= (frames % 10) << 24;
> +            tc |= (s->sei.timecode.seconds_value[i] / 10) << 20;
> +            tc |= (s->sei.timecode.seconds_value[i] % 10) << 16;
> +            tc |= (s->sei.timecode.minutes_value[i] / 10) << 12;
> +            tc |= (s->sei.timecode.minutes_value[i] % 10) << 8;
> +            tc |= (s->sei.timecode.hours_value[i] / 10) << 4;
> +            tc |= (s->sei.timecode.hours_value[i] % 10);
> +
> +            tc_sd[i + 1] = tc;
> +        }
> +
> +        s->sei.timecode.num_clock_ts = 0;
> +    }
> +
>       return 0;
>   }
>   
> 

As you said in the commit message, h264 already does this so it would be 
nice if you didn't duplicate the code. Split both parts of s12m decoding 
you've copied into a common h2645 or just an s12m file maybe?
Lance Wang June 23, 2020, 11:56 p.m. UTC | #2 
On Tue, Jun 23, 2020 at 10:18:55PM +0100, Josh de Kock wrote:
> On 17/06/2020 17:07, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >   libavcodec/hevcdec.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 44 insertions(+)
> > 
> > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> > index c9e28f5..39abbb9 100644
> > --- a/libavcodec/hevcdec.c
> > +++ b/libavcodec/hevcdec.c
> > @@ -2808,6 +2808,50 @@ static int set_side_data(HEVCContext *s)
> >       }
> >       s->sei.unregistered.nb_buf_ref = 0;
> > +    if (s->sei.timecode.present) {
> > +        uint32_t tc = 0;
> > +        uint32_t *tc_sd;
> > +        AVFrameSideData *tcside = av_frame_new_side_data(out, AV_FRAME_DATA_S12M_TIMECODE,
> > +                                                         sizeof(uint32_t) * 4);
> > +        if (!tcside)
> > +            return AVERROR(ENOMEM);
> > +
> > +        tc_sd = (uint32_t*)tcside->data;
> > +        tc_sd[0] = s->sei.timecode.num_clock_ts;
> > +
> > +        for (int i = 0; i < tc_sd[0]; i++) {
> > +            uint32_t frames;
> > +
> > +            /* For SMPTE 12-M timecodes, frame count is a special case if > 30 FPS.
> > +               See SMPTE ST 12-1:2014 Sec 12.1 for more info. */
> > +            if (av_cmp_q(s->avctx->framerate, (AVRational) {30, 1}) == 1) {
> > +                frames = s->sei.timecode.n_frames[i] / 2;
> > +                if (s->sei.timecode.n_frames[i] % 2 == 1) {
> > +                    if (av_cmp_q(s->avctx->framerate, (AVRational) {50, 1}) == 0)
> > +                        tc |= (1 << 7);
> > +                    else
> > +                        tc |= (1 << 23);
> > +                }
> > +            } else {
> > +                frames = s->sei.timecode.n_frames[i];
> > +            }
> > +
> > +            tc |= s->sei.timecode.cnt_dropped_flag[i] << 30;
> > +            tc |= (frames / 10) << 28;
> > +            tc |= (frames % 10) << 24;
> > +            tc |= (s->sei.timecode.seconds_value[i] / 10) << 20;
> > +            tc |= (s->sei.timecode.seconds_value[i] % 10) << 16;
> > +            tc |= (s->sei.timecode.minutes_value[i] / 10) << 12;
> > +            tc |= (s->sei.timecode.minutes_value[i] % 10) << 8;
> > +            tc |= (s->sei.timecode.hours_value[i] / 10) << 4;
> > +            tc |= (s->sei.timecode.hours_value[i] % 10);
> > +
> > +            tc_sd[i + 1] = tc;
> > +        }
> > +
> > +        s->sei.timecode.num_clock_ts = 0;
> > +    }
> > +
> >       return 0;
> >   }
> > 
> 
> As you said in the commit message, h264 already does this so it would be
> nice if you didn't duplicate the code. Split both parts of s12m decoding
> you've copied into a common h2645 or just an s12m file maybe?

Good points, I have considered it and plan to do after the patchset and 
add fate case, then remove the duplicate code to make sure no break case.

I'll add a new function in timecode.h to get tc:
uint32_t av_timecode_get_smpte(tc, fps, drop, ff, ss, mm, hh);

and av_timecode_get_smpte_from_framenum() will use the function to avoid
duplicate code to get smpte.

one question is the bit7 and bit23 isn't set by av_timecode_get_smpte_from_framenum() yet,
I'm not sure whether it'll break any other code if I'll add to the current code.


> 
> -- 
> Josh
> _______________________________________________
> 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".
Lance Wang June 24, 2020, 1:51 p.m. UTC | #3 
On Tue, Jun 23, 2020 at 10:18:55PM +0100, Josh de Kock wrote:
> On 17/06/2020 17:07, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >   libavcodec/hevcdec.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 44 insertions(+)
> > 
> > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> > index c9e28f5..39abbb9 100644
> > --- a/libavcodec/hevcdec.c
> > +++ b/libavcodec/hevcdec.c
> > @@ -2808,6 +2808,50 @@ static int set_side_data(HEVCContext *s)
> >       }
> >       s->sei.unregistered.nb_buf_ref = 0;
> > +    if (s->sei.timecode.present) {
> > +        uint32_t tc = 0;
> > +        uint32_t *tc_sd;
> > +        AVFrameSideData *tcside = av_frame_new_side_data(out, AV_FRAME_DATA_S12M_TIMECODE,
> > +                                                         sizeof(uint32_t) * 4);
> > +        if (!tcside)
> > +            return AVERROR(ENOMEM);
> > +
> > +        tc_sd = (uint32_t*)tcside->data;
> > +        tc_sd[0] = s->sei.timecode.num_clock_ts;
> > +
> > +        for (int i = 0; i < tc_sd[0]; i++) {
> > +            uint32_t frames;
> > +
> > +            /* For SMPTE 12-M timecodes, frame count is a special case if > 30 FPS.
> > +               See SMPTE ST 12-1:2014 Sec 12.1 for more info. */
> > +            if (av_cmp_q(s->avctx->framerate, (AVRational) {30, 1}) == 1) {
> > +                frames = s->sei.timecode.n_frames[i] / 2;
> > +                if (s->sei.timecode.n_frames[i] % 2 == 1) {
> > +                    if (av_cmp_q(s->avctx->framerate, (AVRational) {50, 1}) == 0)
> > +                        tc |= (1 << 7);
> > +                    else
> > +                        tc |= (1 << 23);
> > +                }
> > +            } else {
> > +                frames = s->sei.timecode.n_frames[i];
> > +            }
> > +
> > +            tc |= s->sei.timecode.cnt_dropped_flag[i] << 30;
> > +            tc |= (frames / 10) << 28;
> > +            tc |= (frames % 10) << 24;
> > +            tc |= (s->sei.timecode.seconds_value[i] / 10) << 20;
> > +            tc |= (s->sei.timecode.seconds_value[i] % 10) << 16;
> > +            tc |= (s->sei.timecode.minutes_value[i] / 10) << 12;
> > +            tc |= (s->sei.timecode.minutes_value[i] % 10) << 8;
> > +            tc |= (s->sei.timecode.hours_value[i] / 10) << 4;
> > +            tc |= (s->sei.timecode.hours_value[i] % 10);
> > +
> > +            tc_sd[i + 1] = tc;
> > +        }
> > +
> > +        s->sei.timecode.num_clock_ts = 0;
> > +    }
> > +
> >       return 0;
> >   }
> > 
> 
> As you said in the commit message, h264 already does this so it would be
> nice if you didn't duplicate the code. Split both parts of s12m decoding
> you've copied into a common h2645 or just an s12m file maybe?

I have updated the patchset to remove the code duplication, please help to
review whether it's OK for you, thx.

> 
> -- 
> Josh
> _______________________________________________
> 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/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index c9e28f5..39abbb9 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -2808,6 +2808,50 @@  static int set_side_data(HEVCContext *s)
     }
     s->sei.unregistered.nb_buf_ref = 0;
 
+    if (s->sei.timecode.present) {
+        uint32_t tc = 0;
+        uint32_t *tc_sd;
+        AVFrameSideData *tcside = av_frame_new_side_data(out, AV_FRAME_DATA_S12M_TIMECODE,
+                                                         sizeof(uint32_t) * 4);
+        if (!tcside)
+            return AVERROR(ENOMEM);
+
+        tc_sd = (uint32_t*)tcside->data;
+        tc_sd[0] = s->sei.timecode.num_clock_ts;
+
+        for (int i = 0; i < tc_sd[0]; i++) {
+            uint32_t frames;
+
+            /* For SMPTE 12-M timecodes, frame count is a special case if > 30 FPS.
+               See SMPTE ST 12-1:2014 Sec 12.1 for more info. */
+            if (av_cmp_q(s->avctx->framerate, (AVRational) {30, 1}) == 1) {
+                frames = s->sei.timecode.n_frames[i] / 2;
+                if (s->sei.timecode.n_frames[i] % 2 == 1) {
+                    if (av_cmp_q(s->avctx->framerate, (AVRational) {50, 1}) == 0)
+                        tc |= (1 << 7);
+                    else
+                        tc |= (1 << 23);
+                }
+            } else {
+                frames = s->sei.timecode.n_frames[i];
+            }
+
+            tc |= s->sei.timecode.cnt_dropped_flag[i] << 30;
+            tc |= (frames / 10) << 28;
+            tc |= (frames % 10) << 24;
+            tc |= (s->sei.timecode.seconds_value[i] / 10) << 20;
+            tc |= (s->sei.timecode.seconds_value[i] % 10) << 16;
+            tc |= (s->sei.timecode.minutes_value[i] / 10) << 12;
+            tc |= (s->sei.timecode.minutes_value[i] % 10) << 8;
+            tc |= (s->sei.timecode.hours_value[i] / 10) << 4;
+            tc |= (s->sei.timecode.hours_value[i] % 10);
+
+            tc_sd[i + 1] = tc;
+        }
+
+        s->sei.timecode.num_clock_ts = 0;
+    }
+
     return 0;
 }