Re: [FFmpeg-devel] [PATCH 1/8] libavdevice/decklink: Add support for EIA-708 output over SDI

[Devin Heitmueller]

Hi Aaron,

Comments inline:
> 
> Most of the changes in this set of patches appear to only be relevant in the 
> case that libklvanc is enabled.  Nevertheless, a number of the changes are 
> not wrapped in #if CONFIG_LIBKLVANC, which means that they will be exercised 
> even if libklvanc is not available.  Most of these changes are benign, but 
> the change to use bmdVideoOutputVANC by default may not be benign.  I'm 
> unclear on any side effects of specifying bmdVideoOutputVANC for the output 
> flags instead of bmdVideoOutputFlagDefault.  If this change, for example, 
> exposes a bug in Blackmagic on Windows, where libklvanc is almost certainly 
> not currently available, then this code, which has previously been working on 
> Windows, will suddenly stop working, and for no good reason other than the 
> code is a bit cleaner without #if CONFIG_LIBKLVANC. I'm not saying that such 
> a Blackmagic bug exists (although it wouldn't surprise me if such a bug does 
> exist), but if the user doesn't have libklvanc available when building, as 
> will be the case for almost all users out there, it seems preferable not to 
> expose them to potential issues.  

I’ll do a check and see what is still being used if CONFIG_LIBKLVANC isn’t 
enabled.  Indeed, the focus has been on Linux and OS X, and I haven’t had a 
chance to do any work under Windows (and frankly I don’t really have any plans 
to at this point).

I did make an effort to ensure the code wasn’t being run if the library wasn’t 
present, but I obviously didn’t give it enough attention.

> I think this is also a good argument for having the libklvanc functionality 
> in FFmpeg proper.

There are numerous arguments for why it’s done as a separate library.  Feel 
free to read the responses to the other patch submissions.  I’m happy to 
discuss though in greater detail if desired.

> 
> I'd also argue that, even if CONFIG_LIBKLVANC is available, it should only 
> specify the VANC output flag in the case that VANC will actually be used.  
> Just because VANC is supported doesn't mean it should be used.

This cannot be determined when enabling the video output in cases such as with 
AFD and EIA-708, where the data is announced as side data to the AVFrame (i.e. 
that info isn’t available to us at the write_header phase).  Based on testing 
with a number of versions of the SDK and platforms, enabling it appears to be 
quite safe.  The cases where you get into trouble tend to be when generating 
the actual VANC lines (e.g. overflowing the buffer, etc).
> 
> After this point, assuming that SetAncillaryData() is implemented properly, 
> the reference count for vanc should be 2.  This code owns the "first" 
> reference, and the frame object owns the "second" reference. This code should 
> call Release() on vanc after this point, regardless of the return value of 
> SetAncillaryData().

Ok.  I’ll have to look closer at the reference counting.  I thought it was just 
cribbed from the decklink samples, but I’ll have to better understand what is 
going on here.

>> @@ -393,6 +532,9 @@ av_cold int ff_decklink_write_header(AVFormatContext 
>> *avctx)
>>  ctx->list_formats = cctx->list_formats;
>>  ctx->preroll  = cctx->preroll;
>>  cctx->ctx = ctx;
>> +#if CONFIG_LIBKLVANC
>> +klvanc_context_create(>vanc_ctx);
>> +#endif
> 
> Based on my perusal of the implementation of klvanc_context_create(), the 
> function may fail, yet any failure conditions are not handled here. 
> Admittedly, the only failure case I see in an out of memory error.

Yup, missing an error check.  Will fix.

Devin

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/8] libavdevice/decklink: Add support for EIA-708 output over SDI

[Aaron Levinson]

On 12/29/2017 10:12 AM, Devin Heitmueller wrote:

Hook in libklvanc and use it for output of EIA-708 captions over
SDI.  The bulk of this patch is just general support for ancillary
data for the Decklink SDI module - the real work for construction
of the EIA-708 CDP and VANC line construction is done by libklvanc.

Libklvanc can be found at: https://github.com/stoth68000/libklvanc

Updated to reflect feedback from Marton Balint 

Signed-off-by: Devin Heitmueller 
---
  configure   |   4 ++
  libavcodec/v210enc.c|  10 +++
  libavdevice/decklink_common.cpp |  17 +++--
  libavdevice/decklink_common.h   |  10 +++
  libavdevice/decklink_enc.cpp| 154 ++--
  5 files changed, 185 insertions(+), 10 deletions(-)

diff --git a/configure b/configure
index 6748ef8bc9..19fd04bb6f 100755
--- a/configure
+++ b/configure
@@ -238,6 +238,7 @@ External library support:
--enable-libiec61883 enable iec61883 via libiec61883 [no]
--enable-libilbc enable iLBC de/encoding via libilbc [no]
--enable-libjack enable JACK audio sound server [no]
+  --enable-libklvanc   enable Kernel Labs VANC processing [no]
--enable-libkvazaar  enable HEVC encoding via libkvazaar [no]
--enable-libmodplug  enable ModPlug via libmodplug [no]
--enable-libmp3lame  enable MP3 encoding via libmp3lame [no]
@@ -1602,6 +1603,7 @@ EXTERNAL_LIBRARY_LIST="
  libiec61883
  libilbc
  libjack
+libklvanc
  libkvazaar
  libmodplug
  libmp3lame
@@ -3074,6 +3076,7 @@ decklink_deps_any="libdl LoadLibrary"
  decklink_indev_deps="decklink threads"
  decklink_indev_extralibs="-lstdc++"
  decklink_outdev_deps="decklink threads"
+decklink_outdev_suggest="libklvanc"
  decklink_outdev_extralibs="-lstdc++"
  libndi_newtek_indev_deps="libndi_newtek"
  libndi_newtek_indev_extralibs="-lndi"
@@ -5835,6 +5838,7 @@ enabled libgsm&& { for gsm_hdr in "gsm.h" 
"gsm/gsm.h"; do
 check_lib libgsm "${gsm_hdr}" gsm_create -lgsm 
&& break;
 done || die "ERROR: libgsm not found"; }
  enabled libilbc   && require libilbc ilbc.h WebRtcIlbcfix_InitDecode 
-lilbc $pthreads_extralibs
+enabled libklvanc && require libklvanc libklvanc/vanc.h 
klvanc_context_create -lklvanc
  enabled libkvazaar&& require_pkg_config libkvazaar "kvazaar >= 0.8.1" 
kvazaar.h kvz_api_get
  # While it may appear that require is being used as a pkg-config
  # fallback for libmfx, it is actually being used to detect a different
diff --git a/libavcodec/v210enc.c b/libavcodec/v210enc.c
index a6afbbfc41..51804e3bac 100644
--- a/libavcodec/v210enc.c
+++ b/libavcodec/v210enc.c
@@ -123,6 +123,7 @@ static int encode_frame(AVCodecContext *avctx, AVPacket 
*pkt,
  int aligned_width = ((avctx->width + 47) / 48) * 48;
  int stride = aligned_width * 8 / 3;
  int line_padding = stride - ((avctx->width * 8 + 11) / 12) * 4;
+AVFrameSideData *side_data;
  int h, w, ret;
  uint8_t *dst;
  
@@ -233,6 +234,15 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,

  }
  }
  
+side_data = av_frame_get_side_data(pic, AV_FRAME_DATA_A53_CC);

+if (side_data && side_data->size) {
+uint8_t *buf = av_packet_new_side_data(pkt, AV_PKT_DATA_A53_CC, 
side_data->size);
+if (buf)
+memcpy(buf, side_data->data, side_data->size);
+else
+return AVERROR(ENOMEM);
+}
+
  pkt->flags |= AV_PKT_FLAG_KEY;
  *got_packet = 1;
  return 0;
diff --git a/libavdevice/decklink_common.cpp b/libavdevice/decklink_common.cpp
index 6ef2c529f4..ba091dadea 100644
--- a/libavdevice/decklink_common.cpp
+++ b/libavdevice/decklink_common.cpp
@@ -254,10 +254,19 @@ int ff_decklink_set_format(AVFormatContext *avctx,
 , NULL) != S_OK)
  return -1;
  } else {
-if (ctx->dlo->DoesSupportVideoMode(ctx->bmd_mode, bmdFormat8BitYUV,
-   bmdVideoOutputFlagDefault,
-   , NULL) != S_OK)
-return -1;
+ctx->supports_vanc = 1;
+if (ctx->dlo->DoesSupportVideoMode(ctx->bmd_mode, ctx->raw_format,
+   bmdVideoOutputVANC,
+   , NULL) != S_OK) {
+/* Try again, but without VANC enabled */
+if (ctx->dlo->DoesSupportVideoMode(ctx->bmd_mode, ctx->raw_format,
+   bmdVideoOutputFlagDefault,
+   , NULL) != S_OK) {
+return -1;
+}
+ctx->supports_vanc = 0;
+}
+
  }
  if (support == bmdDisplayModeSupported)
  return 0;
diff --git a/libavdevice/decklink_common.h 

Re: [FFmpeg-devel] [PATCH 1/8] libavdevice/decklink: Add support for EIA-708 output over SDI

[Carl Eugen Hoyos]

2017-12-29 22:00 GMT+01:00 Devin Heitmueller :

> I’m not discounting your concerns - just there isn’t much I can do about
> them but continue to try to improve the quality of the code until it’s
> reliable enough for wider adoption.  :-)

Thank you for the comments!

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/8] libavdevice/decklink: Add support for EIA-708 output over SDI

[Devin Heitmueller]

Hi Carl,

Thanks for your feedback.  Comments inline>

> On Dec 29, 2017, at 3:41 PM, Carl Eugen Hoyos  wrote:
> 
> 2017-12-29 19:12 GMT+01:00 Devin Heitmueller :
> 
>> +side_data = av_frame_get_side_data(pic, AV_FRAME_DATA_A53_CC);
>> +if (side_data && side_data->size) {
>> +uint8_t *buf = av_packet_new_side_data(pkt, AV_PKT_DATA_A53_CC, 
>> side_data->size);
>> +if (buf)
>> +memcpy(buf, side_data->data, side_data->size);
>> +else
>> +return AVERROR(ENOMEM);
> 
> 
> Maybe you disagree but the following is slightly simpler imo:
> 
>  if (!buf)
>return AVERROR();
>  memcpy(buf, data, size);

I don’t feel strongly one way or the other.  If changing it gets it merged, 
then I will resubmit.

> 
> [...]
> 
>> +#if CONFIG_LIBKLVANC
> 
> I tried to voice this before and I assume there is no solution
> but this is a large optional code block depending on an
> external library inside an optional feature depending on
> another - not super-common - external library: This will not
> get much testing, not even for compilation.

It’s a bit of a chicken-and-egg problem.  People who are most likely to use SDI 
are in the broadcast industry, and they typically can’t use ffmpeg in 
production due to all this missing functionality.  My hope is that as we fill 
those holes there will be increased use of the ffmpeg decklink module.

Separating the functionality into a separate library (libklvanc) was 
intentional, as it allows us to use the same VANC processing code across 
multiple open source products (we have versions of VLC and OBE which 
incorporate the library and have been deployed in production for months).  It 
also allows us to share the same code across multiple card manufacturers, 
although there aren’t many to choose from at this point.

I don’t doubt it won’t see much testing at this point given the relatively 
small number of users making use of decklink in ffmpeg.  However that can 
describe a bunch of features in ffmpeg, and I still think there is a lot of 
benefit to getting this stuff upstreamed for those who do want to do SDI 
capture/output.

I’m not discounting your concerns - just there isn’t much I can do about them 
but continue to try to improve the quality of the code until it’s reliable 
enough for wider adoption.  :-)

Devin
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/8] libavdevice/decklink: Add support for EIA-708 output over SDI

[Carl Eugen Hoyos]

2017-12-29 19:12 GMT+01:00 Devin Heitmueller :

> +side_data = av_frame_get_side_data(pic, AV_FRAME_DATA_A53_CC);
> +if (side_data && side_data->size) {
> +uint8_t *buf = av_packet_new_side_data(pkt, AV_PKT_DATA_A53_CC, 
> side_data->size);
> +if (buf)
> +memcpy(buf, side_data->data, side_data->size);
> +else
> +return AVERROR(ENOMEM);


Maybe you disagree but the following is slightly simpler imo:

  if (!buf)
return AVERROR();
  memcpy(buf, data, size);

[...]

> +#if CONFIG_LIBKLVANC

I tried to voice this before and I assume there is no solution
but this is a large optional code block depending on an
external library inside an optional feature depending on
another - not super-common - external library: This will not
get much testing, not even for compilation.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/8] libavdevice/decklink: Add support for EIA-708 output over SDI

[Devin Heitmueller]

Hook in libklvanc and use it for output of EIA-708 captions over
SDI.  The bulk of this patch is just general support for ancillary
data for the Decklink SDI module - the real work for construction
of the EIA-708 CDP and VANC line construction is done by libklvanc.

Libklvanc can be found at: https://github.com/stoth68000/libklvanc

Updated to reflect feedback from Marton Balint 

Signed-off-by: Devin Heitmueller 
---
 configure   |   4 ++
 libavcodec/v210enc.c|  10 +++
 libavdevice/decklink_common.cpp |  17 +++--
 libavdevice/decklink_common.h   |  10 +++
 libavdevice/decklink_enc.cpp| 154 ++--
 5 files changed, 185 insertions(+), 10 deletions(-)

diff --git a/configure b/configure
index 6748ef8bc9..19fd04bb6f 100755
--- a/configure
+++ b/configure
@@ -238,6 +238,7 @@ External library support:
   --enable-libiec61883 enable iec61883 via libiec61883 [no]
   --enable-libilbc enable iLBC de/encoding via libilbc [no]
   --enable-libjack enable JACK audio sound server [no]
+  --enable-libklvanc   enable Kernel Labs VANC processing [no]
   --enable-libkvazaar  enable HEVC encoding via libkvazaar [no]
   --enable-libmodplug  enable ModPlug via libmodplug [no]
   --enable-libmp3lame  enable MP3 encoding via libmp3lame [no]
@@ -1602,6 +1603,7 @@ EXTERNAL_LIBRARY_LIST="
 libiec61883
 libilbc
 libjack
+libklvanc
 libkvazaar
 libmodplug
 libmp3lame
@@ -3074,6 +3076,7 @@ decklink_deps_any="libdl LoadLibrary"
 decklink_indev_deps="decklink threads"
 decklink_indev_extralibs="-lstdc++"
 decklink_outdev_deps="decklink threads"
+decklink_outdev_suggest="libklvanc"
 decklink_outdev_extralibs="-lstdc++"
 libndi_newtek_indev_deps="libndi_newtek"
 libndi_newtek_indev_extralibs="-lndi"
@@ -5835,6 +5838,7 @@ enabled libgsm&& { for gsm_hdr in "gsm.h" 
"gsm/gsm.h"; do
check_lib libgsm "${gsm_hdr}" gsm_create 
-lgsm && break;
done || die "ERROR: libgsm not found"; }
 enabled libilbc   && require libilbc ilbc.h WebRtcIlbcfix_InitDecode 
-lilbc $pthreads_extralibs
+enabled libklvanc && require libklvanc libklvanc/vanc.h 
klvanc_context_create -lklvanc
 enabled libkvazaar&& require_pkg_config libkvazaar "kvazaar >= 0.8.1" 
kvazaar.h kvz_api_get
 # While it may appear that require is being used as a pkg-config
 # fallback for libmfx, it is actually being used to detect a different
diff --git a/libavcodec/v210enc.c b/libavcodec/v210enc.c
index a6afbbfc41..51804e3bac 100644
--- a/libavcodec/v210enc.c
+++ b/libavcodec/v210enc.c
@@ -123,6 +123,7 @@ static int encode_frame(AVCodecContext *avctx, AVPacket 
*pkt,
 int aligned_width = ((avctx->width + 47) / 48) * 48;
 int stride = aligned_width * 8 / 3;
 int line_padding = stride - ((avctx->width * 8 + 11) / 12) * 4;
+AVFrameSideData *side_data;
 int h, w, ret;
 uint8_t *dst;
 
@@ -233,6 +234,15 @@ static int encode_frame(AVCodecContext *avctx, AVPacket 
*pkt,
 }
 }
 
+side_data = av_frame_get_side_data(pic, AV_FRAME_DATA_A53_CC);
+if (side_data && side_data->size) {
+uint8_t *buf = av_packet_new_side_data(pkt, AV_PKT_DATA_A53_CC, 
side_data->size);
+if (buf)
+memcpy(buf, side_data->data, side_data->size);
+else
+return AVERROR(ENOMEM);
+}
+
 pkt->flags |= AV_PKT_FLAG_KEY;
 *got_packet = 1;
 return 0;
diff --git a/libavdevice/decklink_common.cpp b/libavdevice/decklink_common.cpp
index 6ef2c529f4..ba091dadea 100644
--- a/libavdevice/decklink_common.cpp
+++ b/libavdevice/decklink_common.cpp
@@ -254,10 +254,19 @@ int ff_decklink_set_format(AVFormatContext *avctx,
, NULL) != S_OK)
 return -1;
 } else {
-if (ctx->dlo->DoesSupportVideoMode(ctx->bmd_mode, bmdFormat8BitYUV,
-   bmdVideoOutputFlagDefault,
-   , NULL) != S_OK)
-return -1;
+ctx->supports_vanc = 1;
+if (ctx->dlo->DoesSupportVideoMode(ctx->bmd_mode, ctx->raw_format,
+   bmdVideoOutputVANC,
+   , NULL) != S_OK) {
+/* Try again, but without VANC enabled */
+if (ctx->dlo->DoesSupportVideoMode(ctx->bmd_mode, ctx->raw_format,
+   bmdVideoOutputFlagDefault,
+   , NULL) != S_OK) {
+return -1;
+}
+ctx->supports_vanc = 0;
+}
+
 }
 if (support == bmdDisplayModeSupported)
 return 0;
diff --git a/libavdevice/decklink_common.h b/libavdevice/decklink_common.h
index 57ee7d1d68..143bbb951a 100644
--- a/libavdevice/decklink_common.h
+++