[FFmpeg-devel] [PATCH 0/2] Implement SMPTE 2038 output support over Decklink SDI

[FFmpeg-devel] [PATCH 0/2] Implement SMPTE 2038 output support over Decklink SDI

[Devin Heitmueller]

This patch series implements output of SMPTE 2038 VANC over SDI, building
on the prior patch series which added it in the TS domain.  Note that
we moved the AVPacketQueue to be common code within libavdevice so it
can be shared by both the decklink input and output.

Comments/feedback are welcome.

Devin

Devin Heitmueller (2):
  decklink: Move AVPacketQueue into decklink_common
  decklink_enc: add support for SMPTE 2038 VANC packet output

 libavdevice/decklink_common.cpp | 131 ++++++++++++++++++++++++++++++++++++++++
 libavdevice/decklink_common.h   |  11 ++++
 libavdevice/decklink_dec.cpp    | 114 ----------------------------------
 libavdevice/decklink_enc.cpp    | 104 +++++++++++++++++++++++++++++++
 libavdevice/decklink_enc_c.c    |   1 +
 5 files changed, 247 insertions(+), 114 deletions(-)

-- 
1.8.3.1

_______________________________________________
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".

[FFmpeg-devel] [PATCH 1/2] decklink: Move AVPacketQueue into decklink_common

[Devin Heitmueller]

Move the AVPacketQueue functionality that is currently only used
for the decklink decode module into decklink_common, so it can
be shared by the decklink encoder (i.e. for VANC insertion when
we receive data packets separate from video).

Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
---
 libavdevice/decklink_common.cpp | 115 ++++++++++++++++++++++++++++++++++++++++
 libavdevice/decklink_common.h   |   7 +++
 libavdevice/decklink_dec.cpp    | 114 ---------------------------------------
 3 files changed, 122 insertions(+), 114 deletions(-)

diff --git a/libavdevice/decklink_common.cpp b/libavdevice/decklink_common.cpp
index acd1f77..31ab249 100644
--- a/libavdevice/decklink_common.cpp
+++ b/libavdevice/decklink_common.cpp
@@ -390,6 +390,121 @@ int ff_decklink_set_format(AVFormatContext *avctx, decklink_direction_t directio
     return ff_decklink_set_format(avctx, 0, 0, 0, 0, AV_FIELD_UNKNOWN, direction);
 }
 
+void avpacket_queue_init(AVFormatContext *avctx, AVPacketQueue *q)
+{
+    struct decklink_cctx *ctx = (struct decklink_cctx *)avctx->priv_data;
+    memset(q, 0, sizeof(AVPacketQueue));
+    pthread_mutex_init(&q->mutex, NULL);
+    pthread_cond_init(&q->cond, NULL);
+    q->avctx = avctx;
+    q->max_q_size = ctx->queue_size;
+}
+
+void avpacket_queue_flush(AVPacketQueue *q)
+{
+    PacketListEntry *pkt, *pkt1;
+
+    pthread_mutex_lock(&q->mutex);
+    for (pkt = q->pkt_list.head; pkt != NULL; pkt = pkt1) {
+        pkt1 = pkt->next;
+        av_packet_unref(&pkt->pkt);
+        av_freep(&pkt);
+    }
+    q->pkt_list.head = NULL;
+    q->pkt_list.tail = NULL;
+    q->nb_packets = 0;
+    q->size       = 0;
+    pthread_mutex_unlock(&q->mutex);
+}
+
+void avpacket_queue_end(AVPacketQueue *q)
+{
+    avpacket_queue_flush(q);
+    pthread_mutex_destroy(&q->mutex);
+    pthread_cond_destroy(&q->cond);
+}
+
+unsigned long long avpacket_queue_size(AVPacketQueue *q)
+{
+    unsigned long long size;
+    pthread_mutex_lock(&q->mutex);
+    size = q->size;
+    pthread_mutex_unlock(&q->mutex);
+    return size;
+}
+
+int avpacket_queue_put(AVPacketQueue *q, AVPacket *pkt)
+{
+    PacketListEntry *pkt1;
+
+    // Drop Packet if queue size is > maximum queue size
+    if (avpacket_queue_size(q) > (uint64_t)q->max_q_size) {
+        av_packet_unref(pkt);
+        av_log(q->avctx, AV_LOG_WARNING,  "Decklink input buffer overrun!\n");
+        return -1;
+    }
+    /* ensure the packet is reference counted */
+    if (av_packet_make_refcounted(pkt) < 0) {
+        av_packet_unref(pkt);
+        return -1;
+    }
+
+    pkt1 = (PacketListEntry *)av_malloc(sizeof(*pkt1));
+    if (!pkt1) {
+        av_packet_unref(pkt);
+        return -1;
+    }
+    av_packet_move_ref(&pkt1->pkt, pkt);
+    pkt1->next = NULL;
+
+    pthread_mutex_lock(&q->mutex);
+
+    if (!q->pkt_list.tail) {
+        q->pkt_list.head = pkt1;
+    } else {
+        q->pkt_list.tail->next = pkt1;
+    }
+
+    q->pkt_list.tail = pkt1;
+    q->nb_packets++;
+    q->size += pkt1->pkt.size + sizeof(*pkt1);
+
+    pthread_cond_signal(&q->cond);
+
+    pthread_mutex_unlock(&q->mutex);
+    return 0;
+}
+
+int avpacket_queue_get(AVPacketQueue *q, AVPacket *pkt, int block)
+{
+    int ret;
+
+    pthread_mutex_lock(&q->mutex);
+
+    for (;; ) {
+        PacketListEntry *pkt1 = q->pkt_list.head;
+        if (pkt1) {
+            q->pkt_list.head = pkt1->next;
+            if (!q->pkt_list.head) {
+                q->pkt_list.tail = NULL;
+            }
+            q->nb_packets--;
+            q->size -= pkt1->pkt.size + sizeof(*pkt1);
+            *pkt     = pkt1->pkt;
+            av_free(pkt1);
+            ret = 1;
+            break;
+        } else if (!block) {
+            ret = 0;
+            break;
+        } else {
+            pthread_cond_wait(&q->cond, &q->mutex);
+        }
+    }
+    pthread_mutex_unlock(&q->mutex);
+    return ret;
+}
+
 int ff_decklink_list_devices(AVFormatContext *avctx,
                              struct AVDeviceInfoList *device_list,
                              int show_inputs, int show_outputs)
diff --git a/libavdevice/decklink_common.h b/libavdevice/decklink_common.h
index 088e165..d4330e5 100644
--- a/libavdevice/decklink_common.h
+++ b/libavdevice/decklink_common.h
@@ -232,4 +232,11 @@ int ff_decklink_list_formats(AVFormatContext *avctx, decklink_direction_t direct
 void ff_decklink_cleanup(AVFormatContext *avctx);
 int ff_decklink_init_device(AVFormatContext *avctx, const char* name);
 
+void avpacket_queue_init(AVFormatContext *avctx, AVPacketQueue *q);
+void avpacket_queue_flush(AVPacketQueue *q);
+void avpacket_queue_end(AVPacketQueue *q);
+unsigned long long avpacket_queue_size(AVPacketQueue *q);
+int avpacket_queue_put(AVPacketQueue *q, AVPacket *pkt);
+int avpacket_queue_get(AVPacketQueue *q, AVPacket *pkt, int block);
+
 #endif /* AVDEVICE_DECKLINK_COMMON_H */
diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
index 7bf5e37..b3ff2b0 100644
--- a/libavdevice/decklink_dec.cpp
+++ b/libavdevice/decklink_dec.cpp
@@ -471,120 +471,6 @@ skip_packet:
     return tgt;
 }
 
-static void avpacket_queue_init(AVFormatContext *avctx, AVPacketQueue *q)
-{
-    struct decklink_cctx *ctx = (struct decklink_cctx *)avctx->priv_data;
-    memset(q, 0, sizeof(AVPacketQueue));
-    pthread_mutex_init(&q->mutex, NULL);
-    pthread_cond_init(&q->cond, NULL);
-    q->avctx = avctx;
-    q->max_q_size = ctx->queue_size;
-}
-
-static void avpacket_queue_flush(AVPacketQueue *q)
-{
-    PacketListEntry *pkt, *pkt1;
-
-    pthread_mutex_lock(&q->mutex);
-    for (pkt = q->pkt_list.head; pkt != NULL; pkt = pkt1) {
-        pkt1 = pkt->next;
-        av_packet_unref(&pkt->pkt);
-        av_freep(&pkt);
-    }
-    q->pkt_list.head = NULL;
-    q->pkt_list.tail = NULL;
-    q->nb_packets = 0;
-    q->size       = 0;
-    pthread_mutex_unlock(&q->mutex);
-}
-
-static void avpacket_queue_end(AVPacketQueue *q)
-{
-    avpacket_queue_flush(q);
-    pthread_mutex_destroy(&q->mutex);
-    pthread_cond_destroy(&q->cond);
-}
-
-static unsigned long long avpacket_queue_size(AVPacketQueue *q)
-{
-    unsigned long long size;
-    pthread_mutex_lock(&q->mutex);
-    size = q->size;
-    pthread_mutex_unlock(&q->mutex);
-    return size;
-}
-
-static int avpacket_queue_put(AVPacketQueue *q, AVPacket *pkt)
-{
-    PacketListEntry *pkt1;
-
-    // Drop Packet if queue size is > maximum queue size
-    if (avpacket_queue_size(q) > (uint64_t)q->max_q_size) {
-        av_packet_unref(pkt);
-        av_log(q->avctx, AV_LOG_WARNING,  "Decklink input buffer overrun!\n");
-        return -1;
-    }
-    /* ensure the packet is reference counted */
-    if (av_packet_make_refcounted(pkt) < 0) {
-        av_packet_unref(pkt);
-        return -1;
-    }
-
-    pkt1 = (PacketListEntry *)av_malloc(sizeof(*pkt1));
-    if (!pkt1) {
-        av_packet_unref(pkt);
-        return -1;
-    }
-    av_packet_move_ref(&pkt1->pkt, pkt);
-    pkt1->next = NULL;
-
-    pthread_mutex_lock(&q->mutex);
-
-    if (!q->pkt_list.tail) {
-        q->pkt_list.head = pkt1;
-    } else {
-        q->pkt_list.tail->next = pkt1;
-    }
-
-    q->pkt_list.tail = pkt1;
-    q->nb_packets++;
-    q->size += pkt1->pkt.size + sizeof(*pkt1);
-
-    pthread_cond_signal(&q->cond);
-
-    pthread_mutex_unlock(&q->mutex);
-    return 0;
-}
-
-static int avpacket_queue_get(AVPacketQueue *q, AVPacket *pkt, int block)
-{
-    int ret;
-
-    pthread_mutex_lock(&q->mutex);
-
-    for (;; ) {
-        PacketListEntry *pkt1 = q->pkt_list.head;
-        if (pkt1) {
-            q->pkt_list.head = pkt1->next;
-            if (!q->pkt_list.head) {
-                q->pkt_list.tail = NULL;
-            }
-            q->nb_packets--;
-            q->size -= pkt1->pkt.size + sizeof(*pkt1);
-            *pkt     = pkt1->pkt;
-            av_free(pkt1);
-            ret = 1;
-            break;
-        } else if (!block) {
-            ret = 0;
-            break;
-        } else {
-            pthread_cond_wait(&q->cond, &q->mutex);
-        }
-    }
-    pthread_mutex_unlock(&q->mutex);
-    return ret;
-}
 
 static void handle_klv(AVFormatContext *avctx, decklink_ctx *ctx, IDeckLinkVideoInputFrame *videoFrame, int64_t pts)
 {
-- 
1.8.3.1

_______________________________________________
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".

[FFmpeg-devel] [PATCH 2/2] decklink_enc: add support for SMPTE 2038 VANC packet output

[Devin Heitmueller]

Support decoding and embedding VANC packets delivered via SMPTE 2038
into the SDI output.  We leverage an intermediate queue because
data packets are announced separately from video but we need to embed
the data into the video frame when it is output.

Note that this patch has some additional abstraction for data
streams in general as opposed to just SMPTE 2038 packets.  This is
because subsequent patches will introduce support for other
data codecs.

Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
---
 libavdevice/decklink_common.cpp |  16 +++++++
 libavdevice/decklink_common.h   |   4 ++
 libavdevice/decklink_enc.cpp    | 104 ++++++++++++++++++++++++++++++++++++++++
 libavdevice/decklink_enc_c.c    |   1 +
 4 files changed, 125 insertions(+)

diff --git a/libavdevice/decklink_common.cpp b/libavdevice/decklink_common.cpp
index 31ab249..8d2c6d0 100644
--- a/libavdevice/decklink_common.cpp
+++ b/libavdevice/decklink_common.cpp
@@ -505,6 +505,22 @@ int avpacket_queue_get(AVPacketQueue *q, AVPacket *pkt, int block)
     return ret;
 }
 
+int64_t avpacket_queue_peekpts(AVPacketQueue *q)
+{
+    PacketListEntry *pkt1;
+    int64_t pts = -1;
+
+    pthread_mutex_lock(&q->mutex);
+    pkt1 = q->pkt_list.head;
+    if (pkt1) {
+        pts = pkt1->pkt.pts;
+    }
+    pthread_mutex_unlock(&q->mutex);
+
+    return pts;
+}
+
+
 int ff_decklink_list_devices(AVFormatContext *avctx,
                              struct AVDeviceInfoList *device_list,
                              int show_inputs, int show_outputs)
diff --git a/libavdevice/decklink_common.h b/libavdevice/decklink_common.h
index d4330e5..99afcd4 100644
--- a/libavdevice/decklink_common.h
+++ b/libavdevice/decklink_common.h
@@ -112,6 +112,9 @@ struct decklink_ctx {
     /* Capture buffer queue */
     AVPacketQueue queue;
 
+    /* Output VANC queue */
+    AVPacketQueue vanc_queue;
+
     /* Streams present */
     int audio;
     int video;
@@ -238,5 +241,6 @@ void avpacket_queue_end(AVPacketQueue *q);
 unsigned long long avpacket_queue_size(AVPacketQueue *q);
 int avpacket_queue_put(AVPacketQueue *q, AVPacket *pkt);
 int avpacket_queue_get(AVPacketQueue *q, AVPacket *pkt, int block);
+int64_t avpacket_queue_peekpts(AVPacketQueue *q);
 
 #endif /* AVDEVICE_DECKLINK_COMMON_H */
diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp
index 92bfdb2..d7357a0 100644
--- a/libavdevice/decklink_enc.cpp
+++ b/libavdevice/decklink_enc.cpp
@@ -326,6 +326,25 @@ static int create_s337_payload(AVPacket *pkt, uint8_t **outbuf, int *outsize)
     return 0;
 }
 
+static int decklink_setup_data(AVFormatContext *avctx, AVStream *st)
+{
+    int ret = -1;
+
+    switch(st->codecpar->codec_id) {
+#if CONFIG_LIBKLVANC
+    case AV_CODEC_ID_SMPTE_2038:
+        /* No specific setup required */
+        ret = 0;
+        break;
+#endif
+    default:
+        av_log(avctx, AV_LOG_ERROR, "Unsupported data codec specified\n");
+        break;
+    }
+
+    return ret;
+}
+
 av_cold int ff_decklink_write_trailer(AVFormatContext *avctx)
 {
     struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
@@ -351,6 +370,7 @@ av_cold int ff_decklink_write_trailer(AVFormatContext *avctx)
 #if CONFIG_LIBKLVANC
     klvanc_context_destroy(ctx->vanc_ctx);
 #endif
+    avpacket_queue_end(&ctx->vanc_queue);
 
     av_freep(&cctx->ctx);
 
@@ -516,6 +536,58 @@ static int decklink_construct_vanc(AVFormatContext *avctx, struct decklink_ctx *
     construct_cc(avctx, ctx, pkt, &vanc_lines);
     construct_afd(avctx, ctx, pkt, &vanc_lines, st);
 
+    /* See if there any pending data packets to process */
+    while (avpacket_queue_size(&ctx->vanc_queue) > 0) {
+        AVStream *vanc_st;
+        AVPacket vanc_pkt;
+        int64_t pts;
+
+        pts = avpacket_queue_peekpts(&ctx->vanc_queue);
+        if (pts > ctx->last_pts) {
+            /* We haven't gotten to the video frame we are supposed to inject
+               the oldest VANC packet into yet, so leave it on the queue... */
+            break;
+        }
+
+        avpacket_queue_get(&ctx->vanc_queue, &vanc_pkt, 1);
+        if (vanc_pkt.pts + 1 < ctx->last_pts) {
+            av_log(avctx, AV_LOG_WARNING, "VANC packet too old, throwing away\n");
+            av_packet_unref(&vanc_pkt);
+            continue;
+        }
+
+        vanc_st = avctx->streams[vanc_pkt.stream_index];
+        if (vanc_st->codecpar->codec_id == AV_CODEC_ID_SMPTE_2038) {
+            struct klvanc_smpte2038_anc_data_packet_s *pkt_2038 = 0;
+
+            klvanc_smpte2038_parse_pes_payload(vanc_pkt.data, vanc_pkt.size, &pkt_2038);
+            if (pkt_2038 == NULL) {
+                av_log(avctx, AV_LOG_ERROR, "failed to decode SMPTE 2038 PES packet");
+                av_packet_unref(&vanc_pkt);
+                continue;
+            }
+            for (int i = 0; i < pkt_2038->lineCount; i++) {
+                struct klvanc_smpte2038_anc_data_line_s *l = &pkt_2038->lines[i];
+                uint16_t *vancWords = NULL;
+                uint16_t vancWordCount;
+
+                if (klvanc_smpte2038_convert_line_to_words(l, &vancWords,
+                                                           &vancWordCount) < 0)
+                    break;
+
+                ret = klvanc_line_insert(ctx->vanc_ctx, &vanc_lines, vancWords,
+                                         vancWordCount, l->line_number, 0);
+                free(vancWords);
+                if (ret != 0) {
+                    av_log(avctx, AV_LOG_ERROR, "VANC line insertion failed\n");
+                    break;
+                }
+            }
+            klvanc_smpte2038_anc_data_packet_free(pkt_2038);
+        }
+        av_packet_unref(&vanc_pkt);
+    }
+
     IDeckLinkVideoFrameAncillary *vanc;
     int result = ctx->dlo->CreateAncillaryData(bmdFormat10BitYUV, &vanc);
     if (result != S_OK) {
@@ -704,6 +776,23 @@ static int decklink_write_audio_packet(AVFormatContext *avctx, AVPacket *pkt)
     return ret;
 }
 
+static int decklink_write_data_packet(AVFormatContext *avctx, AVPacket *pkt)
+{
+    struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
+    struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx;
+
+    AVPacket *avpacket = av_packet_clone(pkt);
+    if (!avpacket) {
+        av_log(avctx, AV_LOG_ERROR, "Could not clone data packet.\n");
+        return AVERROR(EIO);
+    }
+    if (avpacket_queue_put(&ctx->vanc_queue, avpacket) < 0) {
+        av_log(avctx, AV_LOG_WARNING, "Failed to queue DATA packet\n");
+    }
+
+    return 0;
+}
+
 extern "C" {
 
 av_cold int ff_decklink_write_header(AVFormatContext *avctx)
@@ -768,12 +857,24 @@ av_cold int ff_decklink_write_header(AVFormatContext *avctx)
         } else if (c->codec_type == AVMEDIA_TYPE_VIDEO) {
             if (decklink_setup_video(avctx, st))
                 goto error;
+        } else if (c->codec_type == AVMEDIA_TYPE_DATA) {
+            if (decklink_setup_data(avctx, st))
+                goto error;
         } else {
             av_log(avctx, AV_LOG_ERROR, "Unsupported stream type.\n");
             goto error;
         }
     }
 
+    /* Reconfigure the data stream clocks to match the video */
+    for (n = 0; n < avctx->nb_streams; n++) {
+        AVStream *st = avctx->streams[n];
+        AVCodecParameters *c = st->codecpar;
+        if (c->codec_type == AVMEDIA_TYPE_DATA)
+            avpriv_set_pts_info(st, 64, ctx->bmd_tb_num, ctx->bmd_tb_den);
+    }
+    avpacket_queue_init (avctx, &ctx->vanc_queue);
+
     return 0;
 
 error:
@@ -789,6 +890,9 @@ int ff_decklink_write_packet(AVFormatContext *avctx, AVPacket *pkt)
         return decklink_write_video_packet(avctx, pkt);
     else if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO)
         return decklink_write_audio_packet(avctx, pkt);
+    else if (st->codecpar->codec_type == AVMEDIA_TYPE_DATA) {
+        return decklink_write_data_packet(avctx, pkt);
+    }
 
     return AVERROR(EIO);
 }
diff --git a/libavdevice/decklink_enc_c.c b/libavdevice/decklink_enc_c.c
index f7e3150..7aab660 100644
--- a/libavdevice/decklink_enc_c.c
+++ b/libavdevice/decklink_enc_c.c
@@ -32,6 +32,7 @@ static const AVOption options[] = {
     { "list_devices", "use ffmpeg -sinks decklink instead", OFFSET(list_devices), AV_OPT_TYPE_BOOL, { .i64 = 0   }, 0, 1, ENC | AV_OPT_FLAG_DEPRECATED},
     { "list_formats", "list supported formats"  , OFFSET(list_formats), AV_OPT_TYPE_INT   , { .i64 = 0   }, 0, 1, ENC },
     { "preroll"     , "video preroll in seconds", OFFSET(preroll     ), AV_OPT_TYPE_DOUBLE, { .dbl = 0.5 }, 0, 5, ENC },
+    { "queue_size",   "output queue buffer size", OFFSET(queue_size  ), AV_OPT_TYPE_INT64, { .i64 = (1024 * 1024)}, 0, INT64_MAX, ENC },
 #if BLACKMAGIC_DECKLINK_API_VERSION >= 0x0b000000
     { "duplex_mode" , "duplex mode"             , OFFSET(duplex_mode ), AV_OPT_TYPE_INT   , { .i64 = 0   }, 0, 5, ENC, "duplex_mode"},
 #else
-- 
1.8.3.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 0/2] Implement SMPTE 2038 output support over Decklink SDI

[Marton Balint]

On Fri, 21 Apr 2023, Devin Heitmueller wrote:

> This patch series implements output of SMPTE 2038 VANC over SDI, building
> on the prior patch series which added it in the TS domain.  Note that
> we moved the AVPacketQueue to be common code within libavdevice so it
> can be shared by both the decklink input and output.
>
> Comments/feedback are welcome.

In general, queueing packets in specific components should be avoided if 
possible. Muxed packets are normally ordered by DTS and stream id, generic 
code ensures that. If you want something other than that, then I think 
the perferred way of doing it is by providing a custom interleave 
function. (e.g. to ensure you get data packets before video even if data 
stream has a higher stream ID.)

If you are only using the queue to store multiple data packets for a 
single frame then one way to avoid it is to parse them as soon as they 
arrive via the KLV library. If you insist on queueing them (maybe because 
not every packet will be parased by the KLV lib), then I'd rather see you 
use avpriv_packet_list_*() functions, and not a custom decklink
implementation.

Regards,
Marton
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 0/2] Implement SMPTE 2038 output support over Decklink SDI

[Devin Heitmueller]

Hello Marton,

Thanks for reviewing.  Comments inline:

On Sun, Apr 23, 2023 at 2:43 PM Marton Balint <cus@passwd.hu> wrote:
> In general, queueing packets in specific components should be avoided if
> possible. Muxed packets are normally ordered by DTS and stream id, generic
> code ensures that. If you want something other than that, then I think
> the perferred way of doing it is by providing a custom interleave
> function. (e.g. to ensure you get data packets before video even if data
> stream has a higher stream ID.)

To be clear, using a queue was not first choice.  It's the result of
trying different approaches, and I'm open to constructive suggestions
on alternatives.

While what you're are saying is correct "in general", there are some
really important reasons why it doesn't work in this case.  Permit me
to explain...

By default, the behavior of the mux interleaver is to wait until there
is at least one packet available for each stream before writing to the
output module (in this case decklink).  However data formats such as
SMPTE ST2038 are considered to be "sparse" as there isn't necessarily
a continuous stream of packets like with video and audio (there may be
many seconds between packets, or no packets at all).  As a result you
can't wait for a packet to be available on all streams since on some
streams it will simply wait continuously until hitting the
max_interleave_delta, at which point it will burst out everything in
the queue.  This would cause stalls and/or stuttering playback on the
decklink output.

To accommodate these sparse streams we added code to mux.c to not wait
for 2038 packets.  A side-effect of that though is that packets will
be sent through as soon as they hit the mux, which in most cases will
be significantly ahead of the video (potentially hundreds of
milliseconds).  This can easily be seen experimentally by adding an
av_log() line to ff_decklink_write_packet(), which will show in many
cases the PTS values of the data frames being sent 20+ frames before
the corresponding video.

The queue is there because the data packets and video frames arrive in
separate calls to write_packet(), and they need to be combined to
ensure they are inserted into the same video frame.  Stashing the data
packets seemed like a reasonable approach, and a queue seemed like a
good choice as a data structure since there can be multiple data
packets for a video frame and we might receive data packets for
multiple video frames before the corresponding video frames arrived.

The notion you mentioned that the data packets might arrive after the
video frames is a valid concern hypothetically.  In practice it hasn't
been an issue, as the data packets tend to arrive long before the
video.  It was not a motivation for using a queue.  If a data packet
did arrive after the video (due to the DTS and stream ID ordering you
mentioned), the implementation would insert it on the next video frame
and it would effectively be one frame late.  I was willing to accept
this edge case given it doesn't actually happen in practice.

> If you are only using the queue to store multiple data packets for a
> single frame then one way to avoid it is to parse them as soon as they
> arrive via the KLV library. If you insist on queueing them (maybe because
> not every packet will be parased by the KLV lib), then I'd rather see you
> use avpriv_packet_list_*() functions, and not a custom decklink
> implementation.

Passing them off to libklvanc doesn't actually change the queueing
problem.  The libklvanc library doesn't actually output the VANC
packets but rather just converts them into the byte sequences that
then need to be embedded into the video frames.  I guess I could queue
the output of libklvanc rather than the original AVPackets, but that
doesn't actually solve any of the problems described above, and
actually makes things more complicated since the AVPackets contain all
the timing data and the VANC byte blobs would need to queue not just
the data but also the output timing, VANC line number and horizontal
position within the VANC region.

Regarding the use of avpriv_packet_list() as opposed to
avpacket_queue_*, I used the avpacket_queue functions for consistency
with the decklink capture module where it is used today.  Also,
avpacket_queue is threadsafe while avpriv_packet_list.*() is not.
While the threadsafeness is not critical for the VANC case, I have
subsequent patches for audio where it is important, and I figured it
would more consistent to use the same queue mechanism within decklink
for all three (capture, audio output, and vanc output).

That said, I wouldn't specifically object to converting to the
avpriv_packet_list functions since thread-safeness isn't really a
requirement for this particular case.  It's probably worth noting
though that I extended the avpacket_queue method to allow me to peek
at the first packet in the queue (which avpriv_packet_list doesn't
support today).  Hence converting to avpriv_packet_list would require
an equivalent addition to be accepted upstream.

Devin

--
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com  e: devin.heitmueller@ltnglobal.com
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 0/2] Implement SMPTE 2038 output support over Decklink SDI

[Marton Balint]

On Mon, 24 Apr 2023, Devin Heitmueller wrote:

> Hello Marton,
>
> Thanks for reviewing.  Comments inline:
>
> On Sun, Apr 23, 2023 at 2:43 PM Marton Balint <cus@passwd.hu> wrote:

[...]

Thanks for the detailed explanations. I guess then keeping the queue is 
well justified here.

> Regarding the use of avpriv_packet_list() as opposed to
> avpacket_queue_*, I used the avpacket_queue functions for consistency
> with the decklink capture module where it is used today.  Also,
> avpacket_queue is threadsafe while avpriv_packet_list.*() is not.
> While the threadsafeness is not critical for the VANC case, I have
> subsequent patches for audio where it is important, and I figured it
> would more consistent to use the same queue mechanism within decklink
> for all three (capture, audio output, and vanc output).

Can you explain how thread safety will be relevant for audio? The 
muxer should get packets in a thread safe way, so I don't quite see how 
suddenly it will be needed.

>
> That said, I wouldn't specifically object to converting to the
> avpriv_packet_list functions since thread-safeness isn't really a
> requirement for this particular case.  It's probably worth noting
> though that I extended the avpacket_queue method to allow me to peek
> at the first packet in the queue (which avpriv_packet_list doesn't
> support today).  Hence converting to avpriv_packet_list would require
> an equivalent addition to be accepted upstream.

You can access the internals of the PacketList struct, so you can just add 
needed function to your own code, you don't necessarily have to make it 
public. On the other hand, the avpriv_packet_list does not have the 
concept of queue size or queue count, so it is not only thread safety 
that will be missing.

Two things bother me with the decklink queue:

1) It duplicates the functionality of avpriv_packet_list_put and 
avpriv_packet_list_get, but it seems to me it should not be difficult 
to actually use these get/put functions in the decklink queue as well, 
because it is already using the same packet list struct internally.
Maybe can you give it a try?

2) Namespacing of the struct / functions are wrong. Struct is called 
AVPacketQueue, it should be something like DecklinkPacketQueue in order 
to make it clear that it is not a public struct. The function names are 
prefixed with avpacket, which is also wrong. It should be simply 
packet_queue_xxx, av* would imply a public function. And if you 
factorize it to a non-static function, then it should be 
ff_decklink_packet_queue_xxx.

With these two things fixed, things would look a lot better :)

Regards,
Marton
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 0/2] Implement SMPTE 2038 output support over Decklink SDI

[Devin Heitmueller]

Hi Marton,

Sorry, I'm now recognizing I should have answered this email prior to
the later one.  Comments inline:

On Tue, Apr 25, 2023 at 5:59 PM Marton Balint <cus@passwd.hu> wrote:
> > Regarding the use of avpriv_packet_list() as opposed to
> > avpacket_queue_*, I used the avpacket_queue functions for consistency
> > with the decklink capture module where it is used today.  Also,
> > avpacket_queue is threadsafe while avpriv_packet_list.*() is not.
> > While the threadsafeness is not critical for the VANC case, I have
> > subsequent patches for audio where it is important, and I figured it
> > would more consistent to use the same queue mechanism within decklink
> > for all three (capture, audio output, and vanc output).
>
> Can you explain how thread safety will be relevant for audio? The
> muxer should get packets in a thread safe way, so I don't quite see how
> suddenly it will be needed.

I have a subsequent patch which supports multiple audio output streams
(which may be a mix of PCM and compressed audio).  Those streams need
to be interleaved together before submitting them to the hardware.  I
made a fundamental change to the design such that I employ an
intermediate FIFO which contains the interleaved audio, and the
submission to the hardware is done in the audio callback as we get
close to the scheduling deadline (which runs on a separate thread and
thus the queue needs to be thread-safe).

I am quite confident that considerable discussion will be needed to
explain why I arrived at this design decision, as even I will
acknowledge that it seems ugly at first inspection.  The design has
actually evolved three or four times over the last five years as I had
to address a variety of edge cases found in real-world usage and
working in low-latency environments.

> > That said, I wouldn't specifically object to converting to the
> > avpriv_packet_list functions since thread-safeness isn't really a
> > requirement for this particular case.  It's probably worth noting
> > though that I extended the avpacket_queue method to allow me to peek
> > at the first packet in the queue (which avpriv_packet_list doesn't
> > support today).  Hence converting to avpriv_packet_list would require
> > an equivalent addition to be accepted upstream.
>
> You can access the internals of the PacketList struct, so you can just add
> needed function to your own code, you don't necessarily have to make it
> public. On the other hand, the avpriv_packet_list does not have the
> concept of queue size or queue count, so it is not only thread safety
> that will be missing.

Ok.

> Two things bother me with the decklink queue:
>
> 1) It duplicates the functionality of avpriv_packet_list_put and
> avpriv_packet_list_get, but it seems to me it should not be difficult
> to actually use these get/put functions in the decklink queue as well,
> because it is already using the same packet list struct internally.
> Maybe can you give it a try?

Sure, I can take a look.  I am definitely in favor of using common
functions, and it wasn't until I looked more closely at the code did I
recognize why the author wrote yet another FIFO implementation rather
than using one of the standard public ones.

If we can end up with the decklink queue being a simple wrapper around
avpriv_packet_list() but with an added mutex, then I think that would
be ideal.

> 2) Namespacing of the struct / functions are wrong. Struct is called
> AVPacketQueue, it should be something like DecklinkPacketQueue in order
> to make it clear that it is not a public struct. The function names are
> prefixed with avpacket, which is also wrong. It should be simply
> packet_queue_xxx, av* would imply a public function. And if you
> factorize it to a non-static function, then it should be
> ff_decklink_packet_queue_xxx.

I never really liked the naming either, and agree that it implies the
functionality is public rather than private to decklink.  I can submit
a patch renaming the functions.

Devin

-- 
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com  e: devin.heitmueller@ltnglobal.com
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 0/2] Implement SMPTE 2038 output support over Decklink SDI

[Marton Balint]

On Mon, 24 Apr 2023, Devin Heitmueller wrote:

> Hello Marton,
>
> Thanks for reviewing.  Comments inline:
>
> On Sun, Apr 23, 2023 at 2:43 PM Marton Balint <cus@passwd.hu> wrote:
>> In general, queueing packets in specific components should be avoided if
>> possible. Muxed packets are normally ordered by DTS and stream id, generic
>> code ensures that. If you want something other than that, then I think
>> the perferred way of doing it is by providing a custom interleave
>> function. (e.g. to ensure you get data packets before video even if data
>> stream has a higher stream ID.)
>
> To be clear, using a queue was not first choice.  It's the result of
> trying different approaches, and I'm open to constructive suggestions
> on alternatives.
>
> While what you're are saying is correct "in general", there are some
> really important reasons why it doesn't work in this case.  Permit me
> to explain...
>
> By default, the behavior of the mux interleaver is to wait until there
> is at least one packet available for each stream before writing to the
> output module (in this case decklink).  However data formats such as
> SMPTE ST2038 are considered to be "sparse" as there isn't necessarily
> a continuous stream of packets like with video and audio (there may be
> many seconds between packets, or no packets at all).  As a result you
> can't wait for a packet to be available on all streams since on some
> streams it will simply wait continuously until hitting the
> max_interleave_delta, at which point it will burst out everything in
> the queue.  This would cause stalls and/or stuttering playback on the
> decklink output.
>
> To accommodate these sparse streams we added code to mux.c to not wait
> for 2038 packets.  A side-effect of that though is that packets will
> be sent through as soon as they hit the mux, which in most cases will
> be significantly ahead of the video (potentially hundreds of
> milliseconds).  This can easily be seen experimentally by adding an
> av_log() line to ff_decklink_write_packet(), which will show in many
> cases the PTS values of the data frames being sent 20+ frames before
> the corresponding video.

Okay, I realized there is one thing here I don't understand. What if we 
interleave data packets the same way as others, but we don't wait for them 
in order to start flushing packet queues?

So I wonder, if you removed the AV_CODEC_ID_SMPTE_2038 exception
from init_muxer when calculating si->nb_interleaved_streams but keep the 
exception in ff_interleave_packet_per_dts, and set 
max_interleave_delta to 1, would that work?

Regards,
Marton
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 0/2] Implement SMPTE 2038 output support over Decklink SDI

[Devin Heitmueller]

Hello Marton,

On Wed, Apr 26, 2023 at 3:36 AM Marton Balint <cus@passwd.hu> wrote:
> Okay, I realized there is one thing here I don't understand. What if we
> interleave data packets the same way as others, but we don't wait for them
> in order to start flushing packet queues?
>
> So I wonder, if you removed the AV_CODEC_ID_SMPTE_2038 exception
> from init_muxer when calculating si->nb_interleaved_streams but keep the
> exception in ff_interleave_packet_per_dts, and set
> max_interleave_delta to 1, would that work?

I was actually wondering the same thing after our email exchange
yesterday.  I haven't tried it yet, but I suspect it might very well
result in the 2038 packets not being very far ahead of the video.  We
still need an intermediate data structure to hold onto the 2038
packets (and there could be multiple) before the corresponding video
frame arrives, and a queue is still a reasonable data structure to
store those packets within the decklink module.

Your suggestion might be a good one, and it might change the behavior
such that the packets in general would be more often held in the mux
queue rather than the decklink queue.  But I don't think it changes
anything about the fundamental design, and it doesn't eliminate the
need for stashing the data packets until the corresponding video is to
be sent out.

Devin

-- 
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com  e: devin.heitmueller@ltnglobal.com
_______________________________________________
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".