diff mbox series

[FFmpeg-devel,1/3] ffmpeg: explicitly handle sub2video subpicture initialization

Message ID 20200311234244.28303-1-jeebjp@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/3] ffmpeg: explicitly handle sub2video subpicture initialization | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Jan Ekström March 11, 2020, 11:42 p.m. UTC 
Each time the sub2video structure is initialized, the sub2video
subpicture is initialized together with the first received heartbeat.
The heartbeat's PTS is utilized as the subpicture start time.

Additionally, add some documentation on the stages.
---
 fftools/ffmpeg.c        | 22 +++++++++++++++-------
 fftools/ffmpeg.h        |  3 ++-
 fftools/ffmpeg_filter.c |  8 +++++++-
 3 files changed, 24 insertions(+), 9 deletions(-)

Comments

Jan Ekström March 15, 2020, 6:37 p.m. UTC | #1 
On Thu, Mar 12, 2020 at 1:42 AM Jan Ekström <jeebjp@gmail.com> wrote:
>
> Each time the sub2video structure is initialized, the sub2video
> subpicture is initialized together with the first received heartbeat.
> The heartbeat's PTS is utilized as the subpicture start time.
>
> Additionally, add some documentation on the stages.
> ---
>  fftools/ffmpeg.c        | 22 +++++++++++++++-------
>  fftools/ffmpeg.h        |  3 ++-
>  fftools/ffmpeg_filter.c |  8 +++++++-
>  3 files changed, 24 insertions(+), 9 deletions(-)
>

This patch was originally posted on Feb 26, 2019. It was then pinged
3rd of March, 2019
(https://patchwork.ffmpeg.org/project/ffmpeg/patch/20190226001220.27888-1-jeebjp@gmail.com/).
I decided then to not push it in due to lack of tests for the exact
case which Michael mentioned.

Thus while I anyone is free to review this and I would be much obliged
for such, now that:
- this thing has FATE tests at least making sure that anything which
touches the results of those would be noticed (and then either the
tests or the code gets adjusted depending on which is correct)
- the PGS subtitle sample was uploaded yesterday into the FATE system.

I would like to start pushing this set in during tomorrow, barring
anyone finding issues with the code until then.

Best regards,
Jan
Nicolas George March 15, 2020, 7:06 p.m. UTC | #2 
Jan Ekström (12020-03-15):
> This patch was originally posted on Feb 26, 2019. It was then pinged
> 3rd of March, 2019
> (https://patchwork.ffmpeg.org/project/ffmpeg/patch/20190226001220.27888-1-jeebjp@gmail.com/).
> I decided then to not push it in due to lack of tests for the exact
> case which Michael mentioned.

Sorry, I intented to review this series, but things were a little
hectic. I'll get on with it soon.

Regards,
Nicolas George March 16, 2020, 9:52 a.m. UTC | #3 
Jan Ekström (12020-03-12):
> Each time the sub2video structure is initialized, the sub2video
> subpicture is initialized together with the first received heartbeat.
> The heartbeat's PTS is utilized as the subpicture start time.
> 
> Additionally, add some documentation on the stages.
> ---
>  fftools/ffmpeg.c        | 22 +++++++++++++++-------
>  fftools/ffmpeg.h        |  3 ++-
>  fftools/ffmpeg_filter.c |  8 +++++++-
>  3 files changed, 24 insertions(+), 9 deletions(-)

I think it's ok. No objections for the tests, but I can't upload FATE
samples.

Thanks, and sorry for the delay.

Regards,
Jan Ekström March 16, 2020, 3:59 p.m. UTC | #4 
On Mon, Mar 16, 2020 at 11:52 AM Nicolas George <george@nsup.org> wrote:
>
> Jan Ekström (12020-03-12):
> > Each time the sub2video structure is initialized, the sub2video
> > subpicture is initialized together with the first received heartbeat.
> > The heartbeat's PTS is utilized as the subpicture start time.
> >
> > Additionally, add some documentation on the stages.
> > ---
> >  fftools/ffmpeg.c        | 22 +++++++++++++++-------
> >  fftools/ffmpeg.h        |  3 ++-
> >  fftools/ffmpeg_filter.c |  8 +++++++-
> >  3 files changed, 24 insertions(+), 9 deletions(-)
>
> I think it's ok. No objections for the tests, but I can't upload FATE
> samples.
>
> Thanks, and sorry for the delay.
>

Cheers, thanks for having a look.

The sample was already uploaded on Saturday so I think we're in a
pretty good situation with regards to that.

Will start pushing the patch set soon.

- Jan
diff mbox series

Patch

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index e5fbd479a8..aaaf241314 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -237,7 +237,7 @@  static void sub2video_push_ref(InputStream *ist, int64_t pts)
     }
 }
 
-void sub2video_update(InputStream *ist, AVSubtitle *sub)
+void sub2video_update(InputStream *ist, int64_t heartbeat_pts, AVSubtitle *sub)
 {
     AVFrame *frame = ist->sub2video.frame;
     int8_t *dst;
@@ -254,7 +254,12 @@  void sub2video_update(InputStream *ist, AVSubtitle *sub)
                                  AV_TIME_BASE_Q, ist->st->time_base);
         num_rects = sub->num_rects;
     } else {
-        pts       = ist->sub2video.end_pts;
+        /* If we are initializing the system, utilize current heartbeat
+           PTS as the start time, and show until the following subpicture
+           is received. Otherwise, utilize the previous subpicture's end time
+           as the fall-back value. */
+        pts       = ist->sub2video.initialize ?
+                    heartbeat_pts : ist->sub2video.end_pts;
         end_pts   = INT64_MAX;
         num_rects = 0;
     }
@@ -269,6 +274,7 @@  void sub2video_update(InputStream *ist, AVSubtitle *sub)
         sub2video_copy_rect(dst, dst_linesize, frame->width, frame->height, sub->rects[i]);
     sub2video_push_ref(ist, pts);
     ist->sub2video.end_pts = end_pts;
+    ist->sub2video.initialize = 0;
 }
 
 static void sub2video_heartbeat(InputStream *ist, int64_t pts)
@@ -291,9 +297,11 @@  static void sub2video_heartbeat(InputStream *ist, int64_t pts)
         /* do not send the heartbeat frame if the subtitle is already ahead */
         if (pts2 <= ist2->sub2video.last_pts)
             continue;
-        if (pts2 >= ist2->sub2video.end_pts ||
-            (!ist2->sub2video.frame->data[0] && ist2->sub2video.end_pts < INT64_MAX))
-            sub2video_update(ist2, NULL);
+        if (pts2 >= ist2->sub2video.end_pts || ist2->sub2video.initialize)
+            /* if we have hit the end of the current displayed subpicture,
+               or if we need to initialize the system, update the
+               overlayed subpicture and its start/end times */
+            sub2video_update(ist2, pts2 + 1, NULL);
         for (j = 0, nb_reqs = 0; j < ist2->nb_filters; j++)
             nb_reqs += av_buffersrc_get_nb_failed_requests(ist2->filters[j]->filter);
         if (nb_reqs)
@@ -307,7 +315,7 @@  static void sub2video_flush(InputStream *ist)
     int ret;
 
     if (ist->sub2video.end_pts < INT64_MAX)
-        sub2video_update(ist, NULL);
+        sub2video_update(ist, INT64_MAX, NULL);
     for (i = 0; i < ist->nb_filters; i++) {
         ret = av_buffersrc_add_frame(ist->filters[i]->filter, NULL);
         if (ret != AVERROR_EOF && ret < 0)
@@ -2507,7 +2515,7 @@  static int transcode_subtitles(InputStream *ist, AVPacket *pkt, int *got_output,
         return ret;
 
     if (ist->sub2video.frame) {
-        sub2video_update(ist, &subtitle);
+        sub2video_update(ist, INT64_MIN, &subtitle);
     } else if (ist->nb_filters) {
         if (!ist->sub2video.sub_queue)
             ist->sub2video.sub_queue = av_fifo_alloc(8 * sizeof(AVSubtitle));
diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
index c0b8eb599f..fbaae15377 100644
--- a/fftools/ffmpeg.h
+++ b/fftools/ffmpeg.h
@@ -348,6 +348,7 @@  typedef struct InputStream {
         AVFifoBuffer *sub_queue;    ///< queue of AVSubtitle* before filter init
         AVFrame *frame;
         int w, h;
+        unsigned int initialize; ///< marks if sub2video_update should force an initialization
     } sub2video;
 
     int dr1;
@@ -645,7 +646,7 @@  int filtergraph_is_simple(FilterGraph *fg);
 int init_simple_filtergraph(InputStream *ist, OutputStream *ost);
 int init_complex_filtergraph(FilterGraph *fg);
 
-void sub2video_update(InputStream *ist, AVSubtitle *sub);
+void sub2video_update(InputStream *ist, int64_t heartbeat_pts, AVSubtitle *sub);
 
 int ifilter_parameters_from_frame(InputFilter *ifilter, const AVFrame *frame);
 
diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
index 40cc4c191c..b66faa50b5 100644
--- a/fftools/ffmpeg_filter.c
+++ b/fftools/ffmpeg_filter.c
@@ -740,6 +740,12 @@  static int sub2video_prepare(InputStream *ist, InputFilter *ifilter)
         return AVERROR(ENOMEM);
     ist->sub2video.last_pts = INT64_MIN;
     ist->sub2video.end_pts  = INT64_MIN;
+
+    /* sub2video structure has been (re-)initialized.
+       Mark it as such so that the system will be
+       initialized with the first received heartbeat. */
+    ist->sub2video.initialize = 1;
+
     return 0;
 }
 
@@ -1168,7 +1174,7 @@  int configure_filtergraph(FilterGraph *fg)
             while (av_fifo_size(ist->sub2video.sub_queue)) {
                 AVSubtitle tmp;
                 av_fifo_generic_read(ist->sub2video.sub_queue, &tmp, sizeof(tmp), NULL);
-                sub2video_update(ist, &tmp);
+                sub2video_update(ist, INT64_MIN, &tmp);
                 avsubtitle_free(&tmp);
             }
         }