[vlc-devel] [PATCH] access module for BlackMagic SDI cards

[Jean-Baptiste Kempf]

Hello,

On Mon, Sep 27, 2010 at 01:15:36AM +0200, Steinar H. Gunderson wrote :
> Here's a driver for the BlackMagic DeckLink series of SDI cards (input only).
> It requires BlackMagic's proprietary (but free-to-download) SDK to compile
> and run. I've only tested it on Linux -- the Windows API is very similar but
> not identical, so some adjustments will probably have to happen if anybody
> wants this to work on Windows. (Windows users can, AFAIK, already access the
> card via DirectShow, though.) It supports multiple cards, all the various
> A/V inputs and video modes including setting field dominance and multichannel
> input (2, 8 or 16 channels).
Nice.

> +dnl special access module for BlackMagic SDI cards
> +dnl
> +AC_ARG_ENABLE(sdi,
> +  [  --enable-sdi            BlackMagic SDI access module (default disabled)])
Why default disabled and not default automatic detection of headers?

> diff --git a/modules/access/Modules.am b/modules/access/Modules.am
> index 0b9f94a..0539577 100644
> --- a/modules/access/Modules.am
> +++ b/modules/access/Modules.am
> @@ -52,6 +52,7 @@ SOURCES_access_avio = avio.c avio.h
>  SOURCES_access_attachment = attachment.c
>  SOURCES_access_vdr = vdr.c
>  SOURCES_libbluray = bluray.c
> +SOURCES_sdi = sdi.cpp
>  
>  SOURCES_access_rar = rar/rar.c rar/rar.h rar/access.c
>  SOURCES_stream_filter_rar = rar/rar.c rar/rar.h rar/stream.c

missing po/POTFILES.in entry, missing NEWS and modules/LIST entries.

> +#ifndef INT64_C
> +#define INT64_C(c) c ## LL
> +#endif
Is this necessary ?

> +#include "DeckLinkAPI.h"
> +#include "DeckLinkAPIDispatch.cpp"
Shouldn't this be < > ?

> +    /* Note: AddRef() and Release() here are not thread safe. */
This is an issue, no?

> +HRESULT DeckLinkCaptureDelegate::VideoInputFrameArrived(IDeckLinkVideoInputFrame* videoFrame, IDeckLinkAudioInputPacket* audioFrame)
> +{
> +
> +        p_video_frame = block_New( p_demux_, i_width * i_height * i_bpp );
> +        if( !p_video_frame )
> +        {
> +            msg_Err( p_demux_, "Could not allocate memory for video frame" );
> +            return S_OK;
Is this normal?

> +    if( audioFrame )
> +    {
> +        const int i_bytes = audioFrame->GetSampleFrameCount() * sizeof(int16_t) * p_sys->i_channels;
> +
> +        p_audio_frame = block_New( p_demux_, i_bytes );
> +        if( !p_audio_frame )
> +        {
> +            msg_Err( p_demux_, "Could not allocate memory for audio frame" );
> +            return S_OK;

Same as above. Could this leak p_video_frame?

> +static int Open( vlc_object_t *p_this )
> +{
> +    demux_t     *p_demux = (demux_t*)p_this;
> +    demux_sys_t *p_sys;
> +
> +    /* Only when selected */
> +    if( *p_demux->psz_access == '\0' )
> +        return VLC_EGENERIC;
Are you strict enough?

> +    int i_card_index = var_CreateGetInteger( p_demux, "sdi-card-index" );
Shouldn't you use inherit here?

> +    strcpy(sz_display_mode_padded, "    ");
strncpy, maybe?

> +            p_display_mode->Release();
> +            p_display_iterator->Release();
> +            Close( p_this );
> +            return VLC_EGENERIC;
Can't you factor a bit thise code? like a goto error ?

> +        msg_Err( p_demux, "Failed to start streams" );
What does this mean?

Beware of trailing spaces in this patch

Best Regards,

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/
+33 672 704 734