diff mbox series

[FFmpeg-devel,15/15] fftools/sync_queue: make sure non-limiting streams are not used as queue head

Message ID 20230523135842.20388-15-anton@khirnov.net
State Accepted
Commit 163e3a299e1cc06f0f871d8140def974757e4a7d
Headers show
Series [FFmpeg-devel,01/15] fftools/ffmpeg_hw: move hw_device_setup_for_decode() to ffmpeg_dec | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Anton Khirnov May 23, 2023, 1:58 p.m. UTC 
A non-limiting stream could mistakenly end up being the queue head,
which would then produce incorrect synchronization, seen e.g. in
fate-matroska-flac-extradata-update for certain number of frame threads
(e.g. 5).

Found-By: James Almer
---
 fftools/sync_queue.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Comments

James Almer May 24, 2023, 12:01 p.m. UTC | #1 
On 5/23/2023 10:58 AM, Anton Khirnov wrote:
> A non-limiting stream could mistakenly end up being the queue head,
> which would then produce incorrect synchronization, seen e.g. in
> fate-matroska-flac-extradata-update for certain number of frame threads
> (e.g. 5).
> 
> Found-By: James Almer

Strictly speaking, it was FATE.

> ---
>   fftools/sync_queue.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fftools/sync_queue.c b/fftools/sync_queue.c
> index c0f33e9235..bc107ba4fe 100644
> --- a/fftools/sync_queue.c
> +++ b/fftools/sync_queue.c
> @@ -217,17 +217,26 @@ static void finish_stream(SyncQueue *sq, unsigned int stream_idx)
>   
>   static void queue_head_update(SyncQueue *sq)
>   {
> +    av_assert0(sq->have_limiting);
> +
>       if (sq->head_stream < 0) {
> +        unsigned first_limiting = UINT_MAX;
> +
>           /* wait for one timestamp in each stream before determining
>            * the queue head */
>           for (unsigned int i = 0; i < sq->nb_streams; i++) {
>               SyncQueueStream *st = &sq->streams[i];
> -            if (st->limiting && st->head_ts == AV_NOPTS_VALUE)
> +            if (!st->limiting)
> +                continue;
> +            if (st->head_ts == AV_NOPTS_VALUE)
>                   return;
> +            if (first_limiting == UINT_MAX)
> +                first_limiting = i;
>           }
>   
>           // placeholder value, correct one will be found below
> -        sq->head_stream = 0;
> +        av_assert0(first_limiting < UINT_MAX);
> +        sq->head_stream = first_limiting;
>       }
>   
>       for (unsigned int i = 0; i < sq->nb_streams; i++) {

Can confirm it fixes the issue.
diff mbox series

Patch

diff --git a/fftools/sync_queue.c b/fftools/sync_queue.c
index c0f33e9235..bc107ba4fe 100644
--- a/fftools/sync_queue.c
+++ b/fftools/sync_queue.c
@@ -217,17 +217,26 @@  static void finish_stream(SyncQueue *sq, unsigned int stream_idx)
 
 static void queue_head_update(SyncQueue *sq)
 {
+    av_assert0(sq->have_limiting);
+
     if (sq->head_stream < 0) {
+        unsigned first_limiting = UINT_MAX;
+
         /* wait for one timestamp in each stream before determining
          * the queue head */
         for (unsigned int i = 0; i < sq->nb_streams; i++) {
             SyncQueueStream *st = &sq->streams[i];
-            if (st->limiting && st->head_ts == AV_NOPTS_VALUE)
+            if (!st->limiting)
+                continue;
+            if (st->head_ts == AV_NOPTS_VALUE)
                 return;
+            if (first_limiting == UINT_MAX)
+                first_limiting = i;
         }
 
         // placeholder value, correct one will be found below
-        sq->head_stream = 0;
+        av_assert0(first_limiting < UINT_MAX);
+        sq->head_stream = first_limiting;
     }
 
     for (unsigned int i = 0; i < sq->nb_streams; i++) {