[vlc-devel] [PATCH] Decklink output

Rafaël Carré funman at videolan.org
Tue Jan 22 17:42:22 CET 2013


Le 22/01/2013 16:42, Rémi Denis-Courmont a écrit :
> On Tue, 22 Jan 2013 15:55:41 +0100, Rafaël Carré <funman at videolan.org>
> wrote:
>> ---
>>  configure.ac                      |    2 +-
>>  modules/video_output/Modules.am   |    7 +
>>  modules/video_output/decklink.cpp |  792
>>  +++++++++++++++++++++++++++++++++++++
>>  po/POTFILES.in                    |    1 +
>>  4 files changed, 801 insertions(+), 1 deletion(-)
>>  create mode 100644 modules/video_output/decklink.cpp
>>
>> diff --git a/configure.ac b/configure.ac
>> index 9192c10..fdb3e94 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -1764,7 +1764,7 @@ if test "${enable_decklink}" != "no"
>>  then
>>    if test "${with_decklink_sdk}" != "no" -a -n "${with_decklink_sdk}"
>>    then
>> -    VLC_ADD_CXXFLAGS([decklink],[-I${with_decklink_sdk}/include])
>> +    VLC_ADD_CXXFLAGS([decklink
>> decklinkoutput],[-I${with_decklink_sdk}/include])
> 
> This change does not seem to be actually used.

Changed Modules.am to use CXXFLAGS_decklinkoutput

>>    fi
>>    VLC_SAVE_FLAGS
>>    CXXFLAGS="${CXXFLAGS} ${CXXFLAGS_decklink}"
>> diff --git a/modules/video_output/Modules.am
>> b/modules/video_output/Modules.am
>> index a5aa5e9..87357b7 100644
>> --- a/modules/video_output/Modules.am
>> +++ b/modules/video_output/Modules.am
>> @@ -10,6 +10,13 @@ SOURCES_vout_macosx = macosx.m opengl.h opengl.c
>>  SOURCES_vout_ios = ios.m opengl.h opengl.c
>>  SOURCES_android_surface = androidsurface.c
>>  
>> +if HAVE_DECKLINK
>> +libdecklinkoutput_plugin_la_SOURCES = decklink.cpp
>> +libdecklinkoutput_plugin_la_CXXFLAGS = $(AM_CFLAGS)
> $(CXXFLAGS_decklink)
>> +libdecklinkoutput_plugin_la_LIBADD = $(AM_LIBADD) $(LIBS_decklink) -ldl
> 
> $(LIBDL) ?

OK, changed.

>> +libvlc_LTLIBRARIES += libdecklinkoutput_plugin.la
>> +endif
>> +
>>  ### OpenGL ###
>>  # TODO: merge all three source files (?)
>>  libgles2_plugin_la_SOURCES = opengl.c opengl.h gl.c
>> diff --git a/modules/video_output/decklink.cpp
>> b/modules/video_output/decklink.cpp
>> new file mode 100644
>> index 0000000..752b27c
>> --- /dev/null
>> +++ b/modules/video_output/decklink.cpp
>> @@ -0,0 +1,792 @@
>>
> +/*****************************************************************************
>> + * decklink.cpp: BlackMagic DeckLink SDI output module
>> +
>>
> *****************************************************************************
>> + * Copyright (C) 2012-2013 Rafaël Carré
>> + * Copyright (C) 2009 Michael Niedermayer <michaelni at gmx.at>
>> + * Copyright (c) 2009 Baptiste Coudurier <baptiste dot coudurier at
> gmail
>> dot com>
>> + *
>> + * Authors: Rafaël Carré <funman at videolan.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
> it
>> + * under the terms of the GNU Lesser General Public License as
> published
>> by
>> + * the Free Software Foundation; either version 2.1 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> License
>> + * along with this program; if not, write to the Free Software
> Foundation,
>> + * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
>> +
>>
> *****************************************************************************/
>> +
>> +#define __STDC_FORMAT_MACROS
>> +#define __STDC_CONSTANT_MACROS
>> +
>> +#ifdef HAVE_CONFIG_H
>> +# include "config.h"
>> +#endif
>> +
>> +#include <stdint.h>
>> +#include <cassert>
> 
> stdint.h vs cassert... not very consistent.

assert.h that will be

>> +#include <vlc_common.h>
>> +#include <vlc_plugin.h>
>> +#include <vlc_threads.h>
> 
> This is currently an useless include. Do you think it should be removed
> from vlc_common.h?

I think it would not harm to do that, we can use vlc-test.git to see
which files need to include it first.

>> +#include <vlc_vout_display.h>
>> +#include <vlc_picture_pool.h>
>> +
>> +#include <vlc_block.h>
>> +#include <vlc_atomic.h>
>> +#include <vlc_aout.h>
>> +#include <arpa/inet.h>
>> +
>> +#include <DeckLinkAPI.h>
>> +#include <DeckLinkAPIDispatch.cpp>
>> +
>> +#define FRAME_SIZE 1920
>> +#define CHANNELS_MAX 6
>> +
>> +static const int pi_channels_maps[CHANNELS_MAX+1] =
>> +{
>> +    0,
>> +    AOUT_CHAN_CENTER,
>> +    AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT,
>> +    AOUT_CHAN_CENTER | AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT,
>> +    AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT | AOUT_CHAN_REARLEFT
>> +     | AOUT_CHAN_REARRIGHT,
>> +    AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT | AOUT_CHAN_CENTER
>> +     | AOUT_CHAN_REARLEFT | AOUT_CHAN_REARRIGHT,
>> +    AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT | AOUT_CHAN_CENTER
>> +     | AOUT_CHAN_REARLEFT | AOUT_CHAN_REARRIGHT | AOUT_CHAN_LFE
>> +};
> 
> Could use AOUT_CHANS_* macros...

OK, changed.

>> +
>> +#define CARD_INDEX_TEXT N_("Output card to use")
> 
> "to use" is pointless, also below.
> 
>> +#define CARD_INDEX_LONGTEXT N_(\
>> +    "DeckLink output card to use, if multiple exist. " \
>> +    "The cards are numbered from 0.")
>> +
>> +#define MODE_TEXT N_("Desired output video mode")
>> +#define MODE_LONGTEXT N_(\
>> +    "Desired output video mode for DeckLink output. " \
>> +    "This value should be a FOURCC code in textual " \
>> +    "form, e.g. \"ntsc\".")
> 
> s/output video/video/

Done.

>> +
>> +#define AUDIO_CONNECTION_TEXT N_("Audio connection")
>> +#define AUDIO_CONNECTION_LONGTEXT N_(\
>> +    "Audio connection to use for DeckLink output. " \
>> +    "Valid choices: embedded, aesebu, analog. " \
>> +    "Leave blank for card default.")
>> +
>> +
>> +#define RATE_TEXT N_("Audio sampling rate in Hz")
>> +#define RATE_LONGTEXT N_(\
>> +    "Audio sampling rate (in hertz) for DeckLink output. " \
>> +    "0 disables audio output.")
>> +
>> +#define CHANNELS_TEXT N_("Number of audio channels")
>> +#define CHANNELS_LONGTEXT N_(\
>> +    "Number of output audio channels for DeckLink output. " \
>> +    "Must be 2, 8 or 16. 0 disables audio output.")
>> +
>> +#define VIDEO_CONNECTION_TEXT N_("Video connection")
>> +#define VIDEO_CONNECTION_LONGTEXT N_(\
>> +    "Video connection to use for DeckLink output. " \
>> +    "Valid choices: sdi, hdmi, opticalsdi, component, " \
>> +    "composite, svideo. " \
>> +    "Leave blank for card default.")
> 
> That won't work in typical preferences UI.
> You should have a default choice in the list instead.

The list is already here, I removed the choices from help text and chose
a sane default

>> +
>> +#define CFG_PREFIX "decklink-output-"
>> +#define VIDEO_CFG_PREFIX "decklink-vout-"
>> +#define AUDIO_CFG_PREFIX "decklink-aout-"
>> +
>> +
>> +
>> +static const char *const ppsz_videoconns[] = {
>> +    "sdi", "hdmi", "opticalsdi", "component", "composite", "svideo"
>> +};
>> +static const char *const ppsz_videoconns_text[] = {
>> +    N_("SDI"), N_("HDMI"), N_("Optical SDI"), N_("Component"),
>> N_("Composite"), N_("S-video")
>> +};
>> +
>> +static const char *const ppsz_audioconns[] = {
>> +    "embedded", "aesebu", "analog"
>> +};
>> +static const char *const ppsz_audioconns_text[] = {
>> +    N_("Embedded"), N_("AES/EBU"), N_("Analog")
>> +};
>> +
>> +
>> +struct vout_display_sys_t
>> +{
>> +    picture_pool_t *pool;
>> +    bool tenbits;
>> +};
>> +
>> +static struct
>> +{
>> +    IDeckLink *p_card;
>> +    IDeckLinkOutput *p_output;
>> +    IDeckLinkConfiguration *p_config;
>> +    IDeckLinkDisplayModeIterator *p_display_iterator;
>> +    IDeckLinkIterator *decklink_iterator;
>> +
>> +    int i_channels;
>> +    int i_rate;
>> +
>> +    int i_width;
>> +    int i_height;
>> +
>> +    BMDTimeScale timescale;
>> +    BMDTimeValue frameduration;
>> +
>> +    /* XXX: workaround card clock drift */
>> +    mtime_t offset;
>> +} decklink_sys = {
>> +    NULL, NULL, NULL, NULL, NULL,
>> +    0, 0,
>> +    -1, -1,
>> +    0, 0,
>> +    0,
>> +};
> 
> When you have global writable data, the locking mechanism should be
> obvious and/or commented on.

Added a comment (we allow only one audio and video output modules per
process.)

>>
> +/*****************************************************************************
>> + * Local prototypes.
>> +
>>
> *****************************************************************************/
>> +
>> +static int  OpenVideo           (vlc_object_t *);
>> +static void CloseVideo          (vlc_object_t *);
>> +static int  OpenAudio           (vlc_object_t *);
>> +static void CloseAudio          (vlc_object_t *);
>> +
>>
> +/*****************************************************************************
>> + * Module descriptor
>> +
>>
> *****************************************************************************/
>> +
>> +vlc_module_begin()
>> +    set_shortname(N_("DecklinkOutput"))
>> +    set_description(N_("Decklink Output plug-in"))
>> +    set_section(N_("Decklink General Options"), NULL)
>> +    add_integer(CFG_PREFIX "card-index", 0,
>> +                CARD_INDEX_TEXT, CARD_INDEX_LONGTEXT, true)
>> +
>> +    add_submodule ()
>> +    set_description (N_("Decklink Video Output module"))
>> +    set_category(CAT_VIDEO)
>> +    set_subcategory(SUBCAT_VIDEO_VOUT)
>> +    set_capability("vout display", 0)
>> +    set_callbacks (OpenVideo, CloseVideo)
>> +    set_section(N_("Decklink Video Options"), NULL)
>> +    add_string(VIDEO_CFG_PREFIX "video-connection", 0,
>> +                VIDEO_CONNECTION_TEXT, VIDEO_CONNECTION_LONGTEXT, true)
>> +                change_string_list(ppsz_videoconns,
> ppsz_videoconns_text)
>> +    add_string(VIDEO_CFG_PREFIX "mode", "pal ",
>> +                MODE_TEXT, MODE_LONGTEXT, true)
>> +    add_bool(VIDEO_CFG_PREFIX "tenbits", false, N_("10 bits"), N_("10
>> bits"), true)
> 
> IMHO, this deserves more explicit (long) text.

Agree, this was done hastily : added more explicit text.

>> +
>> +
>> +    add_submodule ()
>> +    set_description (N_("Decklink Audio Output module"))
>> +    set_category(CAT_AUDIO)
>> +    set_subcategory(SUBCAT_AUDIO_AOUT)
>> +    set_capability("audio output", 0)
>> +    set_callbacks (OpenAudio, CloseAudio)
>> +    set_section(N_("Decklink Audio Options"), NULL)
>> +    add_string(AUDIO_CFG_PREFIX "audio-connection", 0,
>> +                AUDIO_CONNECTION_TEXT, AUDIO_CONNECTION_LONGTEXT, true)
>> +                change_string_list(ppsz_audioconns,
> ppsz_audioconns_text)
>> +    add_integer(AUDIO_CFG_PREFIX "audio-rate", 48000,
>> +                RATE_TEXT, RATE_LONGTEXT, true)
>> +    add_integer(AUDIO_CFG_PREFIX "audio-channels", 2,
>> +                CHANNELS_TEXT, CHANNELS_LONGTEXT, true)
>> +vlc_module_end ()
>> +
>> +// Connection mode
>> +static BMDAudioConnection getAConn(vlc_object_t *p_this)
>> +{
>> +    BMDAudioConnection conn = 0;
>> +    char *psz = var_InheritString(p_this, AUDIO_CFG_PREFIX
>> "audio-connection");
>> +    if (!psz)
>> +        goto end;
>> +
>> +    if (!strcmp(psz, "embedded"))
>> +        conn = bmdAudioConnectionEmbedded;
>> +    else if (!strcmp(psz, "aesebu"))
>> +        conn = bmdAudioConnectionAESEBU;
>> +    else if (!strcmp(psz, "analog"))
>> +        conn = bmdAudioConnectionAnalog;
>> +
>> +end:
>> +    free(psz);
>> +    return conn;
>> +}
>> +
>> +static BMDVideoConnection getVConn(vlc_object_t *p_this)
>> +{
>> +    BMDVideoConnection conn = 0;
>> +    char *psz = var_InheritString(p_this, VIDEO_CFG_PREFIX
>> "video-connection");
>> +    if (!psz)
>> +        goto end;
>> +
>> +         if (!strcmp(psz, "sdi"))
>> +        conn = bmdVideoConnectionSDI;
>> +    else if (!strcmp(psz, "hdmi"))
>> +        conn = bmdVideoConnectionHDMI;
>> +    else if (!strcmp(psz, "opticalsdi"))
>> +        conn = bmdVideoConnectionOpticalSDI;
>> +    else if (!strcmp(psz, "component"))
>> +        conn = bmdVideoConnectionComponent;
>> +    else if (!strcmp(psz, "composite"))
>> +        conn = bmdVideoConnectionComposite;
>> +    else if (!strcmp(psz, "svideo"))
>> +        conn = bmdVideoConnectionSVideo;
>> +
>> +end:
>> +    free(psz);
>> +    return conn;
>> +}
>> +
>>
> +/*****************************************************************************
>> + *
>> +
>>
> *****************************************************************************/
>> +
>> +static void CloseDecklink(void)
>> +{
>> +    decklink_sys.p_output->StopScheduledPlayback(0, NULL, 0);
>> +    decklink_sys.p_output->DisableVideoOutput();
>> +    decklink_sys.p_output->DisableAudioOutput();
>> +
>> +    decklink_sys.i_width = -1;
> 
> Suspiciously unsynchronized memory write...

Removed, it should not be needed (but shouldn't have harmed anyway since
this runs in the destructor so no other code should access decklink_sys
at the same time)

>> +
>> +    if (decklink_sys.decklink_iterator)
>> +        decklink_sys.decklink_iterator->Release();
>> +
>> +    if (decklink_sys.p_display_iterator)
>> +        decklink_sys.p_display_iterator->Release();
>> +
>> +    if (decklink_sys.p_config)
>> +        decklink_sys.p_config->Release();
>> +
>> +    if (decklink_sys.p_output)
>> +        decklink_sys.p_output->Release();
>> +
>> +    if (decklink_sys.p_card)
>> +        decklink_sys.p_card->Release();
>> +}
>> +
>> +static int OpenDecklink(vlc_object_t *p_this)
>> +{
>> +    vout_display_t *vd = (vout_display_t *)p_this;
>> +    vout_display_sys_t *sys = vd->sys;
>> +
>> +#define CHECK(message) do { \
>> +    if (result != S_OK) \
>> +    { \
>> +        msg_Err(p_this, message ": %d", result); \
>> +        goto error; \
>> +    } \
>> +} while(0)
> 
> IIRC, HRESULT is usually printed as hexadecimal.

OK

>> +
>> +    HRESULT result;
>> +    IDeckLinkDisplayMode *p_display_mode = NULL;
>> +    static vlc_mutex_t lock = VLC_STATIC_MUTEX;
>> +    static bool initialized = false;
>> +
>> +    vlc_mutex_lock(&lock);
>> +
>> +    if (initialized) {
>> +        /* already initialized */
>> +        vlc_mutex_unlock(&lock);
>> +        return VLC_SUCCESS;
>> +    }
> 
> So can't change the settings without starting a new process!?

No.. more on that below.

>> +
>> +    decklink_sys.i_channels = var_InheritInteger(p_this,
> AUDIO_CFG_PREFIX
>> "audio-channels");
>> +    decklink_sys.i_rate = var_InheritInteger(p_this, AUDIO_CFG_PREFIX
>> "audio-rate");
>> +    int i_card_index = var_InheritInteger(p_this, CFG_PREFIX
>> "card-index");
>> +    BMDVideoConnection vconn = getVConn(p_this);
>> +    BMDAudioConnection aconn = getAConn(p_this);
>> +    char *mode = var_InheritString(p_this, VIDEO_CFG_PREFIX "mode");
> 
> If settings are really process-wide, config_Get*() is to be used.

idem

>> +    size_t len = mode ? strlen(mode) : 0;
>> +    if (!mode || len > 4)
>> +    {
>> +        free(mode);
>> +        msg_Err(p_this, "Missing or invalid mode");
>> +        goto error;
>> +    }
>> +
>> +    BMDDisplayMode wanted_mode_id;
>> +    memset(&wanted_mode_id, ' ', 4);
>> +    strncpy((char*)&wanted_mode_id, mode, 4);
>> +    free(mode);
>> +
>> +    if (i_card_index < 0)
>> +    {
>> +        msg_Err(p_this, "Invalid card index %d", i_card_index);
>> +        goto error;
>> +    }
>> +
>> +    decklink_sys.decklink_iterator = CreateDeckLinkIteratorInstance();
>> +    if (!decklink_sys.decklink_iterator)
>> +    {
>> +        msg_Err(p_this, "DeckLink drivers not found.");
>> +        goto error;
>> +    }
>> +
>> +    for(int i = 0; i <= i_card_index; ++i)
>> +    {
>> +        if (decklink_sys.p_card) {
>> +            //decklink_sys.p_card->Release();
>> +            msg_Dbg(p_this, "Error here %d Cannot Release Pcard dc",
> i);
> 
> I don't know what the problem is. This deserves a comment (or a fix).

Fixed.
It might have been a problem with older decklink SDK.

>> +        }
>> +        result =
>> decklink_sys.decklink_iterator->Next(&decklink_sys.p_card);
>> +        CHECK("Card not found");
>> +    }
>> +
>> +    const char *psz_model_name;
>> +    result = decklink_sys.p_card->GetModelName(&psz_model_name);
>> +    CHECK("Unknown model name");
>> +
>> +    msg_Dbg(p_this, "Opened DeckLink PCI card %s", psz_model_name);
>> +
>> +    result = decklink_sys.p_card->QueryInterface(IID_IDeckLinkOutput,
>> +        (void**)&decklink_sys.p_output);
>> +    CHECK("No outputs");
>> +
>> +    result =
>> decklink_sys.p_card->QueryInterface(IID_IDeckLinkConfiguration,
>> +        (void**)&decklink_sys.p_config);
>> +    CHECK("Could not get config interface");
>> +
>> +    if (vconn)
>> +    {
>> +        result = decklink_sys.p_config->SetInt(
>> +            bmdDeckLinkConfigVideoOutputConnection, vconn);
>> +        CHECK("Could not set video output connection");
>> +    }
>> +
>> +    if (aconn)
>> +    {
>> +        result = decklink_sys.p_config->SetInt(
>> +            bmdDeckLinkConfigAudioInputConnection, aconn);
>> +        CHECK("Could not set audio output connection");
>> +    }
>> +
>> +    result =
>>
> decklink_sys.p_output->GetDisplayModeIterator(&decklink_sys.p_display_iterator);
>> +    CHECK("Could not enumerate display modes");
>> +
>> +    for (; ; p_display_mode->Release())
>> +    {
>> +        int w, h;
>> +        result =
> decklink_sys.p_display_iterator->Next(&p_display_mode);
>> +        if (result != S_OK)
>> +            break;
>> +
>> +        BMDDisplayMode mode_id =
> ntohl(p_display_mode->GetDisplayMode());
>> +
>> +        const char *psz_mode_name;
>> +        result = p_display_mode->GetName(&psz_mode_name);
>> +        CHECK("Could not get display mode name");
>> +
>> +        result =
> p_display_mode->GetFrameRate(&decklink_sys.frameduration,
>> +            &decklink_sys.timescale);
>> +        CHECK("Could not get frame rate");
>> +
>> +        w = p_display_mode->GetWidth();
>> +        h = p_display_mode->GetHeight();
>> +        msg_Dbg(p_this, "Found mode '%4.4s': %s (%dx%d, %.3f fps)",
>> +                (char*)&mode_id, psz_mode_name, w, h, 
>> +                double(decklink_sys.timescale) /
>> decklink_sys.frameduration);
>> +        msg_Dbg(p_this, "scale %d dur %d", (int)decklink_sys.timescale,
>> +            (int)decklink_sys.frameduration);
>> +
>> +        if (wanted_mode_id != mode_id)
>> +            continue;
>> +
>> +        decklink_sys.i_width = w;
>> +        decklink_sys.i_height = h;
>> +
>> +        p_display_mode->Release();
>> +        p_display_mode = NULL;
>> +
>> +        mode_id = htonl(mode_id);
>> +
>> +        BMDVideoOutputFlags flags = bmdVideoOutputVANC;
>> +        if (mode_id == bmdModeNTSC ||
>> +            mode_id == bmdModeNTSC2398 ||
>> +            mode_id == bmdModePAL)
>> +        {
>> +            flags = bmdVideoOutputVITC;
>> +        }
>> +
>> +        BMDDisplayModeSupport support;
>> +        IDeckLinkDisplayMode *resultMode;
>> +
>> +        result = decklink_sys.p_output->DoesSupportVideoMode(mode_id,
>> +            sys->tenbits ? bmdFormat10BitYUV : bmdFormat8BitYUV,
>> +            flags, &support, &resultMode);
>> +        CHECK("Does not support video mode");
>> +        if (support == bmdDisplayModeNotSupported)
>> +        {
>> +            msg_Err(p_this, "Video mode not supported");
>> +                goto error;
>> +        }
>> +
>> +        result = decklink_sys.p_output->EnableVideoOutput(mode_id,
> flags);
>> +        CHECK("Could not enable video output");
>> +
>> +        break;
>> +    }
>> +
>> +    if (decklink_sys.i_width < 0)
>> +    {
>> +        msg_Err(p_this, "Unknown video mode specified.");
>> +        goto error;
>> +    }
>> +
>> +    /* audio */
>> +    if (decklink_sys.i_channels > 0 && decklink_sys.i_rate > 0)
>> +    {
>> +        result = decklink_sys.p_output->EnableAudioOutput(
>> +            decklink_sys.i_rate,
>> +            bmdAudioSampleType16bitInteger,
>> +            decklink_sys.i_channels,
>> +            bmdAudioOutputStreamTimestamped);
>> +    }
>> +    else
>> +    {
>> +        result = decklink_sys.p_output->DisableAudioOutput();
>> +    }
>> +    CHECK("Could not enable audio output");
>> +
>> +
>> +    /* start */
>> +    result = decklink_sys.p_output->StartScheduledPlayback(
>> +        (mdate() * decklink_sys.timescale) / CLOCK_FREQ,
>> decklink_sys.timescale, 1.0);
>> +    CHECK("Could not start playback");
>> +
>> +    atexit(CloseDecklink);
> 
> atexit() is not legal in shared objects, especially dynamically loaded
> ones. If you _really_ need to run exit code, use a destructor function
> instead. Either way, note that you cannot call any VLC functions from the
> exit code.

Agreed that we can't use VLC code, and replaced atexit with
__attribute__((destructor)) (and moved static initialized variable).

If I _really_ need to run on exit code is not sure, I added this hack
after seeing weird crashes inside decklink closed source SDK ...

It *might* have been fixed by Blackmagic but unfortunately I didn't keep
the old code.

>> +    initialized = true;
>> +
>> +    vlc_mutex_unlock(&lock);
>> +    return VLC_SUCCESS;
>> +
>> +
>> +error:
>> +    if (decklink_sys.decklink_iterator)
>> +        decklink_sys.decklink_iterator->Release();
>> +    if (decklink_sys.p_display_iterator)
>> +        decklink_sys.p_display_iterator->Release();
>> +    if (p_display_mode)
>> +        p_display_mode->Release();
>> +
>> +    vlc_mutex_unlock(&lock);
>> +    return VLC_EGENERIC;
>> +#undef CHECK
>> +}
>> +
>>
> +/*****************************************************************************
>> + * Video
>> +
>>
> *****************************************************************************/
>> +
>> +static picture_pool_t *PoolVideo(vout_display_t *vd, unsigned
>> requested_count)
>> +{
>> +    vout_display_sys_t *sys = vd->sys;
>> +    if (!sys->pool)
>> +        sys->pool = picture_pool_NewFromFormat(&vd->fmt,
> requested_count);
>> +    return sys->pool;
>> +}
>> +
>> +static inline void put_le32(uint8_t **p, uint32_t d)
>> +{
>> +    SetDWLE(*p, d);
>> +    (*p) += 4;
>> +}
>> +
>> +static inline int clip(int a)
>> +{
>> +    if      (a < 4) return 4;
>> +    else if (a > 1019) return 1019;
>> +    else               return a;
>> +}
>> +
>> +static void v210_convert(void *frame_bytes, picture_t *pic, int
>> dst_stride)
>> +{
>> +    int width = pic->format.i_width;
>> +    int height = pic->format.i_height;
>> +    int line_padding = dst_stride - ((width * 8 + 11) / 12) * 4;
>> +    int h, w;
>> +    uint8_t *data = (uint8_t*)frame_bytes;
>> +
>> +    const uint16_t *y = (const uint16_t*)pic->p[0].p_pixels;
>> +    const uint16_t *u = (const uint16_t*)pic->p[1].p_pixels;
>> +    const uint16_t *v = (const uint16_t*)pic->p[2].p_pixels;
>> +
>> +#define WRITE_PIXELS(a, b, c)           \
>> +    do {                                \
>> +        val =   clip(*a++);             \
>> +        val |= (clip(*b++) << 10) |     \
>> +               (clip(*c++) << 20);      \
>> +        put_le32(&data, val);           \
>> +    } while (0)
>> +
>> +    for (h = 0; h < height; h++) {
>> +        uint32_t val = 0;
>> +        for (w = 0; w < width - 5; w += 6) {
>> +            WRITE_PIXELS(u, y, v);
>> +            WRITE_PIXELS(y, u, y);
>> +            WRITE_PIXELS(v, y, u);
>> +            WRITE_PIXELS(y, v, y);
>> +        }
>> +        if (w < width - 1) {
>> +            WRITE_PIXELS(u, y, v);
>> +
>> +            val = clip(*y++);
>> +            if (w == width - 2)
>> +                put_le32(&data, val);
>> +#undef WRITE_PIXELS
>> +        }
>> +        if (w < width - 3) {
>> +            val |= (clip(*u++) << 10) | (clip(*y++) << 20);
>> +            put_le32(&data, val);
>> +
>> +            val = clip(*v++) | (clip(*y++) << 10);
>> +            put_le32(&data, val);
>> +        }
>> +
>> +        memset(data, 0, line_padding);
>> +        data += line_padding;
>> +
>> +        y += pic->p[0].i_pitch / 2 - width;
>> +        u += pic->p[1].i_pitch / 2 - width / 2;
>> +        v += pic->p[2].i_pitch / 2 - width / 2;
>> +    }
>> +}
>> +
>> +static void DisplayVideo(vout_display_t *vd, picture_t *picture,
>> subpicture_t *)
>> +{
>> +    vout_display_sys_t *sys = vd->sys;
>> +
>> +    if (!picture)
>> +        return;
>> +
>> +    HRESULT result;
>> +    int w, h, stride, length;
>> +    mtime_t now;
>> +    w = decklink_sys.i_width;
>> +    h = decklink_sys.i_height;
>> +
>> +    IDeckLinkMutableVideoFrame *pDLVideoFrame;
>> +    result = decklink_sys.p_output->CreateVideoFrame(w, h, w*3,
>> +        sys->tenbits ? bmdFormat10BitYUV : bmdFormat8BitYUV,
>> +        bmdFrameFlagDefault, &pDLVideoFrame);
>> +
>> +    if (result != S_OK) {
>> +        msg_Err(vd, "Failed to create video frame: %d", result);
>> +        goto end;
>> +    }
>> +
>> +    void *frame_bytes;
>> +    pDLVideoFrame->GetBytes((void**)&frame_bytes);
>> +    stride = pDLVideoFrame->GetRowBytes();
>> +
>> +    if (sys->tenbits)
>> +        v210_convert(frame_bytes, picture, stride);
>> +    else for(int y = 0; y < h; ++y) {
>> +        uint8_t *dst = (uint8_t *)frame_bytes + stride * y;
>> +        const uint8_t *src = (const uint8_t *)picture->p[0].p_pixels +
>> +            picture->p[0].i_pitch * y;
>> +        memcpy(dst, src, w * 2 /* bpp */);
>> +    }
>> +
>> +
>> +    // compute frame duration in CLOCK_FREQ units
>> +    length = (decklink_sys.frameduration * CLOCK_FREQ) /
>> decklink_sys.timescale;
>> +
>> +    picture->date -= decklink_sys.offset;
>> +    result = decklink_sys.p_output->ScheduleVideoFrame(pDLVideoFrame,
>> +        picture->date, length, CLOCK_FREQ);
>> +
>> +    if (result != S_OK) {
>> +        msg_Err(vd, "Dropped Video frame %"PRId64 "(0x%x)",
>> +            picture->date, result);
>> +        goto end;
>> +    }
>> +
>> +    now = mdate() - decklink_sys.offset;
>> +
>> +    BMDTimeValue decklink_now;
>> +    double speed;
>> +    decklink_sys.p_output->GetScheduledStreamTime (CLOCK_FREQ,
>> &decklink_now, &speed);
>> +
>> +    if ((now - decklink_now) > 400000) {
>> +        /* XXX: workaround card clock drift */
>> +        decklink_sys.offset += 50000;
>> +        msg_Err(vd, "Delaying: offset now %"PRId64"",
>> decklink_sys.offset);
>> +    }
>> +
>> +end:
>> +    pDLVideoFrame->Release();
>> +    picture_Release(picture);
>> +}
>> +
>> +static int ControlVideo(vout_display_t *vd, int query, va_list args)
>> +{
>> +    VLC_UNUSED(vd);
>> +    const vout_display_cfg_t *cfg;
>> +
>> +    switch (query) {
>> +    case VOUT_DISPLAY_CHANGE_FULLSCREEN:
>> +        cfg = va_arg(args, const vout_display_cfg_t *);
>> +        return cfg->is_fullscreen ? VLC_EGENERIC : VLC_SUCCESS;
>> +    default:
>> +        return VLC_EGENERIC;
>> +    }
>> +}
>> +
>> +static vlc_mutex_t video_lock = VLC_STATIC_MUTEX;
>> +static int OpenVideo(vlc_object_t *p_this)
>> +{
>> +    vout_display_t *vd = (vout_display_t *)p_this;
>> +    vout_display_sys_t *sys;
>> +
>> +    if (vlc_mutex_trylock(&video_lock)) {
>> +        msg_Err(vd, "Decklink video module already busy");
>> +        return VLC_EGENERIC;
>> +    }
>> +
>> +    vd->sys = sys = (vout_display_sys_t*)malloc(sizeof(*sys));
>> +    if (!sys)
>> +        return VLC_ENOMEM;
>> +
>> +    if (OpenDecklink(p_this) != VLC_SUCCESS)
>> +        goto error;
>> +
>> +    if (decklink_sys.i_width & 1) {
>> +        msg_Err(vd, "Invalid width %d", decklink_sys.i_width);
>> +        goto error;
>> +    }
>> +
>> +    sys->pool = NULL;
>> +
>> +    sys->tenbits = var_InheritBool(p_this, VIDEO_CFG_PREFIX "tenbits");
>> +    vd->fmt.i_chroma = sys->tenbits
>> +        ? VLC_CODEC_I422_10L /* we will convert to v210 */
>> +        : VLC_CODEC_UYVY;
>> +    //video_format_FixRgb(&(vd->fmt));
>> +
>> +    vd->fmt.i_width = decklink_sys.i_width;
>> +    vd->fmt.i_height = decklink_sys.i_height;
>> +
>> +    vd->info.has_hide_mouse = true;
>> +    vd->pool    = PoolVideo;
>> +    vd->prepare = NULL;
>> +    vd->display = DisplayVideo;
>> +    vd->control = ControlVideo;
>> +    vd->manage  = NULL;
>> +    vout_display_SendEventFullscreen(vd, false);
>> +
>> +    return VLC_SUCCESS;
>> +
>> +error:
>> +    free(sys);
>> +    return VLC_EGENERIC;
>> +}
>> +
>> +static void CloseVideo(vlc_object_t *p_this)
>> +{
>> +    vout_display_t *vd = (vout_display_t *)p_this;
>> +    vout_display_sys_t *sys = vd->sys;
>> +
>> +    if (sys->pool)
>> +        picture_pool_Delete(sys->pool);
>> +
>> +    free(sys);
>> +
>> +    vlc_mutex_unlock(&video_lock);
> 
> There is no warranty that close and open run on the same thread. Well
> maybe the current video output works that way right now, but that's not a
> very safe and future-proof assumption. Thus a semaphore would be more
> appropriate than a mutex.

First I am not sure how semaphores can be used here?
We don't want to block if decklink module is already in use (e.g.
equivalent to trylock), also I see no static constructor.

Also I only added those mutexes today, to prevent e.g. libvlc users to
load 2 libvlc instances in the same process, so I could remove them and
ignore this corner case.
One libvlc instance will work because we can only have one vout and one
aout.

static global data (decklink_sys) is used to synchronize audio and video
output modules atm.
If there is a way to allocate it dynamically at first use and share the
pointer between the 2 modules, we could get rid of these mutexes
(video_lock and audio_lock).
And we could have 2 libvlc instances in one process, talking to 2
different cards.

I am not sure how config_Get helps that?
I was looking for code using var_InheritPointer but this function
doesn't exist .. any hint?

>> +}
>> +
>>
> +/*****************************************************************************
>> + * Audio
>> +
>>
> *****************************************************************************/
>> +
>> +static void Flush (audio_output_t *aout, bool drain)
>> +{
>> +    if (drain) {
>> +        uint32_t samples;
>> +       
> decklink_sys.p_output->GetBufferedAudioSampleFrameCount(&samples);
>> +        msleep(CLOCK_FREQ * samples / decklink_sys.i_rate);
>> +    } else if (decklink_sys.p_output->FlushBufferedAudioSamples() ==
>> E_FAIL)
>> +        msg_Err(aout, "Flush failed");
>> +}
>> +
>> +static int TimeGet(audio_output_t *, mtime_t* restrict)
>> +{
>> +    /* synchronization is handled by the card */
>> +    return -1;
>> +}
>> +
>> +static int Start(audio_output_t *aout, audio_sample_format_t *restrict
>> fmt)
>> +{
>> +    if (OpenDecklink(VLC_OBJECT(aout)) != VLC_SUCCESS)
>> +        return VLC_EGENERIC;
>> +
>> +    fmt->i_format = VLC_CODEC_S16N;
>> +    fmt->i_channels = decklink_sys.i_channels;
>> +    fmt->i_physical_channels = pi_channels_maps[fmt->i_channels];
>> +    fmt->i_rate = decklink_sys.i_rate;
>> +    fmt->i_bitspersample = 16;
>> +    fmt->i_blockalign = fmt->i_channels * fmt->i_bitspersample /8 ;
>> +    fmt->i_frame_length  = FRAME_SIZE;
>> +
>> +    return VLC_SUCCESS;
>> +}
>> +
>> +static void PlayAudio(audio_output_t *aout, block_t *audio)
>> +{
>> +    if (!audio)
>> +        return;
> 
> Impossible, I think.

OK, removed.

>> +
>> +    audio->i_pts -= decklink_sys.offset;
>> +
>> +    uint32_t sampleFrameCount = audio->i_buffer / (2 *
>> decklink_sys.i_channels);
>> +    uint32_t written;
>> +    HRESULT result = decklink_sys.p_output->ScheduleAudioSamples(
>> +            audio->p_buffer, sampleFrameCount, audio->i_pts,
> CLOCK_FREQ,
>> &written);
>> +
>> +    if (result != S_OK)
>> +        msg_Err(aout, "Failed to schedule audio sample: %d", result);
>> +    else if (sampleFrameCount != written)
>> +        msg_Err(aout, "Written only %d samples out of %d", written,
>> sampleFrameCount);
>> +
>> +    block_Release(audio);
>> +}
>> +
>> +static vlc_mutex_t audio_lock = VLC_STATIC_MUTEX;
>> +static int OpenAudio(vlc_object_t *p_this)
>> +{
>> +    audio_output_t *aout = (audio_output_t *)p_this;
>> +
>> +    if (vlc_mutex_trylock(&audio_lock)) {
>> +        msg_Err(aout, "Decklink audio module already busy");
>> +        return VLC_EGENERIC;
>> +    }
>> +
>> +    aout->play      = PlayAudio;
>> +    aout->start     = Start;
>> +    aout->flush     = Flush;
>> +    aout->time_get  = TimeGet;
>> +
>> +    aout->pause     = NULL;
>> +    aout->stop      = NULL;
>> +    aout->mute_set  = NULL;
>> +    aout->volume_set= NULL;
>> +
>> +    return VLC_SUCCESS;
>> +}
>> +
>> +static void CloseAudio(vlc_object_t *)
>> +{
>> +    vlc_mutex_unlock(&audio_lock);
> 
> Same as above.
> 
>> +}
>> diff --git a/po/POTFILES.in b/po/POTFILES.in
>> index 7eb17ea..e1e22b3 100644
>> --- a/po/POTFILES.in
>> +++ b/po/POTFILES.in
>> @@ -1141,6 +1141,7 @@ modules/video_filter/yuvp.c
>>  modules/video_output/aa.c
>>  modules/video_output/androidsurface.c
>>  modules/video_output/caca.c
>> +modules/video_output/decklink.cpp
>>  modules/video_output/directfb.c
>>  modules/video_output/drawable.c
>>  modules/video_output/egl.c
> 
> modules/LIST is missing.
> 

Added.


Thank you for review, I will answer with updated patch



More information about the vlc-devel mailing list