[FFmpeg-devel,3/4] Add support for 10-bit output for Decklink SDI

From: Devin Heitmueller <dheitmueller@kernellabs.com>

Can be tested via the following command:

./ffmpeg -i foo.ts -f decklink -vcodec v210 'DeckLink Duo (1)'

Note that the 8-bit support works as it did before, and setting
the pix_fmt isn't required for 10-bit mode.  The code defaults to
operating in 8-bit mode when no vcodec is specified, for backward
compatibility.

Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
---
 libavdevice/decklink_enc.cpp | 110 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 83 insertions(+), 27 deletions(-)

On Tue, 26 Sep 2017, Devin Heitmueller wrote:

> From: Devin Heitmueller <dheitmueller@kernellabs.com>
>
> Can be tested via the following command:
>
> ./ffmpeg -i foo.ts -f decklink -vcodec v210 'DeckLink Duo (1)'
>
> Note that the 8-bit support works as it did before, and setting
> the pix_fmt isn't required for 10-bit mode.  The code defaults to
> operating in 8-bit mode when no vcodec is specified, for backward
> compatibility.
>
> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
> ---
> libavdevice/decklink_enc.cpp | 110 ++++++++++++++++++++++++++++++++-----------
> 1 file changed, 83 insertions(+), 27 deletions(-)
>
> diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp
> index 0f654faa19..0d965699ec 100644
> --- a/libavdevice/decklink_enc.cpp
> +++ b/libavdevice/decklink_enc.cpp
> @@ -44,20 +44,45 @@ extern "C" {
> class decklink_frame : public IDeckLinkVideoFrame
> {
> public:
> -    decklink_frame(struct decklink_ctx *ctx, AVFrame *avframe) :
> -                   _ctx(ctx), _avframe(avframe),  _refs(1) { }
> -
> -    virtual long           STDMETHODCALLTYPE GetWidth      (void)          { return _avframe->width; }
> -    virtual long           STDMETHODCALLTYPE GetHeight     (void)          { return _avframe->height; }
> -    virtual long           STDMETHODCALLTYPE GetRowBytes   (void)          { return _avframe->linesize[0] < 0 ? -_avframe->linesize[0] : _avframe->linesize[0]; }
> -    virtual BMDPixelFormat STDMETHODCALLTYPE GetPixelFormat(void)          { return bmdFormat8BitYUV; }
> -    virtual BMDFrameFlags  STDMETHODCALLTYPE GetFlags      (void)          { return _avframe->linesize[0] < 0 ? bmdFrameFlagFlipVertical : bmdFrameFlagDefault; }
> +    decklink_frame(struct decklink_ctx *ctx, AVFrame *avframe, AVCodecID codec_id, int height, int width) :
> +        _ctx(ctx), _avframe(avframe), _avpacket(NULL), _codec_id(codec_id), _height(height), _width(width),  _refs(1) { }
> +    decklink_frame(struct decklink_ctx *ctx, AVPacket *avpacket, AVCodecID codec_id, int height, int width) :
> +        _ctx(ctx), _avframe(NULL), _avpacket(avpacket), _codec_id(codec_id), _height(height), _width(width), _refs(1) { }
> +
> +    virtual long           STDMETHODCALLTYPE GetWidth      (void)          { return _width; }
> +    virtual long           STDMETHODCALLTYPE GetHeight     (void)          { return _height; }
> +    virtual long           STDMETHODCALLTYPE GetRowBytes   (void)
> +    {
> +      if (_codec_id == AV_CODEC_ID_WRAPPED_AVFRAME)
> +          return _avframe->linesize[0] < 0 ? -_avframe->linesize[0] : _avframe->linesize[0];
> +      else
> +          return ((GetWidth() + 47) / 48) * 128;
> +    }
> +    virtual BMDPixelFormat STDMETHODCALLTYPE GetPixelFormat(void)
> +    {
> +        if (_codec_id == AV_CODEC_ID_WRAPPED_AVFRAME)
> +            return bmdFormat8BitYUV;
> +        else
> +            return bmdFormat10BitYUV;
> +    }
> +    virtual BMDFrameFlags  STDMETHODCALLTYPE GetFlags      (void)
> +    {
> +       if (_codec_id == AV_CODEC_ID_WRAPPED_AVFRAME)
> +           return _avframe->linesize[0] < 0 ? bmdFrameFlagFlipVertical : bmdFrameFlagDefault;
> +       else
> +           return bmdFrameFlagDefault;
> +    }
> +
>     virtual HRESULT        STDMETHODCALLTYPE GetBytes      (void **buffer)
>     {
> +      if (_avframe) {

Maybe you should check for codec id here as well for consistency.

>         if (_avframe->linesize[0] < 0)
>             *buffer = (void *)(_avframe->data[0] + _avframe->linesize[0] * (_avframe->height - 1));
>         else
>             *buffer = (void *)(_avframe->data[0]);

I think this section is short enough to reindent it in the same patch.

> +      } else {
> +            *buffer = (void *)(_avpacket->data);

The DeckLink SDK requires a 128 byte alignment for data. I am thinking 
AVPacket does not always provides that. Maybe we should simply ignore the 
SDK requirement (if it works without it?) Can you test this somehow?

> +      }
>         return S_OK;
>     }
> 
> @@ -70,7 +95,10 @@ public:
>     {
>         int ret = --_refs;
>         if (!ret) {
> -            av_frame_free(&_avframe);
> +            if (_codec_id == AV_CODEC_ID_WRAPPED_AVFRAME)
> +                av_frame_free(&_avframe);
> +            else
> +                av_packet_unref(_avpacket);

You are using av_packet_unref instead of av_packet_free, is this 
intentional? av_frame_free and av_packet_free can handle NULL pointers, 
so you don't have to check the codec type, just free both.

>             delete this;
>         }
>         return ret;
> @@ -78,6 +106,10 @@ public:
>
>     struct decklink_ctx *_ctx;
>     AVFrame *_avframe;
> +    AVPacket *_avpacket;
> +    AVCodecID _codec_id;
> +    int _height;
> +    int _width;
> 
> private:
>     std::atomic<int>  _refs;
> @@ -92,7 +124,10 @@ public:
>         struct decklink_ctx *ctx = frame->_ctx;
>         AVFrame *avframe = frame->_avframe;
> 
> -        av_frame_unref(avframe);
> +        if (avframe)
> +            av_frame_unref(avframe);
> +        else
> +            av_packet_unref(frame->_avpacket);

Maybe something like this is simpler and easier to follow:

if (frame->_avframe)
     av_frame_unref(frame->_avframe);
if (frame->_avpacket)
     av_packet_unref(frame->_avpacket);

>
>         pthread_mutex_lock(&ctx->mutex);
>         ctx->frames_buffer_available_spots++;
> @@ -118,11 +153,14 @@ static int decklink_setup_video(AVFormatContext *avctx, AVStream *st)
>         return -1;
>     }
> 
> -    if (c->format != AV_PIX_FMT_UYVY422) {
> -        av_log(avctx, AV_LOG_ERROR, "Unsupported pixel format!"
> -               " Only AV_PIX_FMT_UYVY422 is supported.\n");
> -        return -1;
> +    if (c->codec_id == AV_CODEC_ID_WRAPPED_AVFRAME) {
> +        if (c->format != AV_PIX_FMT_UYVY422) {
> +            av_log(avctx, AV_LOG_ERROR, "Unsupported pixel format!"
> +                   " Only AV_PIX_FMT_UYVY422 is supported.\n");
> +            return -1;
> +        }
>     }
> +
>     if (ff_decklink_set_format(avctx, c->width, c->height,
>                             st->time_base.num, st->time_base.den, c->field_order)) {
>         av_log(avctx, AV_LOG_ERROR, "Unsupported video size, framerate or field order!"
> @@ -230,27 +268,45 @@ static int decklink_write_video_packet(AVFormatContext *avctx, AVPacket *pkt)
> {
>     struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
>     struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx;
> -    AVFrame *avframe, *tmp = (AVFrame *)pkt->data;
> +    AVStream *st = avctx->streams[pkt->stream_index];
> +    AVFrame *avframe = NULL, *tmp = (AVFrame *)pkt->data;
> +    AVPacket *avpacket = NULL;
>     decklink_frame *frame;
>     buffercount_type buffered;
>     HRESULT hr;
> 
> -    if (tmp->format != AV_PIX_FMT_UYVY422 ||
> -        tmp->width  != ctx->bmd_width ||
> -        tmp->height != ctx->bmd_height) {
> -        av_log(avctx, AV_LOG_ERROR, "Got a frame with invalid pixel format or dimension.\n");
> -        return AVERROR(EINVAL);
> -    }
> -    avframe = av_frame_clone(tmp);
> -    if (!avframe) {
> -        av_log(avctx, AV_LOG_ERROR, "Could not clone video frame.\n");
> -        return AVERROR(EIO);
> +    if (st->codecpar->codec_id == AV_CODEC_ID_WRAPPED_AVFRAME) {
> +        if (tmp->format != AV_PIX_FMT_UYVY422 ||
> +            tmp->width  != ctx->bmd_width ||
> +            tmp->height != ctx->bmd_height) {
> +            av_log(avctx, AV_LOG_ERROR, "Got a frame with invalid pixel format or dimension.\n");
> +            return AVERROR(EINVAL);
> +        }
> +
> +        avframe = av_frame_clone(tmp);
> +        if (!avframe) {
> +            av_log(avctx, AV_LOG_ERROR, "Could not clone video frame.\n");
> +            return AVERROR(EIO);
> +        }
> +
> +        frame = new decklink_frame(ctx, avframe, st->codecpar->codec_id, avframe->height, avframe->width);
> +    } else {
> +        avpacket = av_packet_clone(pkt);
> +        if (!avpacket) {
> +            av_log(avctx, AV_LOG_ERROR, "Could not clone video frame.\n");
> +            return AVERROR(EIO);
> +        }
> +
> +        frame = new decklink_frame(ctx, avpacket, st->codecpar->codec_id, ctx->bmd_height, ctx->bmd_width);
>     }
> 
> -    frame = new decklink_frame(ctx, avframe);
>     if (!frame) {
>         av_log(avctx, AV_LOG_ERROR, "Could not create new frame.\n");
> -        av_frame_free(&avframe);
> +
> +        if (avframe)
> +            av_frame_free(&avframe);
> +        else
> +            av_packet_unref(avpacket);

av_packet_free here as well.

>         return AVERROR(EIO);
>     }
> 
> -- 
> 2.13.2

Regards,
Marton

Hello Marton,

Thanks for taking the time to provide feedback.

>> +      } else {
>> +            *buffer = (void *)(_avpacket->data);
> 
> The DeckLink SDK requires a 128 byte alignment for data. I am thinking AVPacket does not always provides that. Maybe we should simply ignore the SDK requirement (if it works without it?) Can you test this somehow?

The SDK does expect the stride to be a multiple of 128 (which the v210 codec does), and the SDK does suggest that each line should start on a 128-byte aligned boundary.  I’ve worked with three or four different implementations on both the input and output side, and never had an issue where alignment was a problem.  That said, it’s possible that the issue is present on some less common model decklink card, or perhaps the implementation of some feature internal to the SDK has optimized assembly which expects the alignment.

In any case, it would be a pre-existing bug in libavcodec/v210enc.c, not something specific to the decklink output.

If someone wants to point me to an example of aligned allocation in libavcodec, I can take a look.  I think any such issue though would be separate from the content in this patch.

The other issues you mentioned have been addressed and I’m testing the changes now.  I should have updated patches for you shortly.

Regards,

Devin