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

[Steinar H. Gunderson]

On Sat, Oct 02, 2010 at 01:18:58AM +0200, Laurent Aimar wrote:
>> +/*****************************************************************************
>> + * decklink.cpp: BlackMagic DeckLink SDI input module
>> + *****************************************************************************
>> + * Copyright (C) 2010 VideoLAN
>  Not correct, you can assign it to yourself.

Done.

>> +#ifndef INT64_C
>> +#define INT64_C(c) c ## LL
>> +#endif
> Adding
> #define __STDC_CONSTANT_MACROS 1
> before the config.h include should properly defines it.

Aha, it needs the one. That makes it work, indeed.

>> +#include <vlc_common.h>
>> +#include <vlc_plugin.h>
>> +#include <vlc_input.h>
>> +#include <vlc_demux.h>
>> +#include <vlc_access.h>
>> +#include <vlc_picture.h>
>> +#include <vlc_charset.h>
>> +#include <vlc_fs.h>
>> +#include <vlc_atomic.h>
>> +#include <arpa/inet.h>
>  Are you sure you need all of thoses headers ? (At quick glance,
>  vlc_access, vlc_picture are probably not needed, I haven't check the
> others).

Removed about half of them (input, access, picture, charset, fs).

>> +struct demux_sys_t
>> +{
>> +    IDeckLink *p_card;
>> +    IDeckLinkInput *p_input;
>> +    DeckLinkCaptureDelegate *p_delegate;
>> +
>> +    es_out_id_t *p_video_es;
>> +    es_out_id_t *p_audio_es;
>> +    bool b_first_frame;
>> +    int i_last_pts;
>> +
>> +    int i_width, i_height, i_fps_num, i_fps_den;
>  Do you need them here ?

No, not anymore. Moved.

>> +    uint32_t i_dominance_flags;
>> +
>> +    int i_rate, i_channels;
>  Same.

We need i_channels (see below).

>> +private:
>> +    vlc_atomic_t m_ref_;
>> +    demux_t *p_demux_;
>  Why the _ suffix ?

It's just a convention for saying “member variable”. There are many different
conventions in use, though; do you want me to use another?

>> +HRESULT DeckLinkCaptureDelegate::VideoInputFormatChanged(BMDVideoInputFormatChangedEvents events, IDeckLinkDisplayMode *mode, BMDDetectedVideoInputFormatFlags)
>> +{
>> +    msg_Dbg( p_demux_, "Video input format changed" );
>> +    return S_OK;
>> +}
>  Could be moved to the class definition (it's a one line).

Done.

>> +            const uint8_t *src = (const uint8_t *)frame_bytes + i_stride * y;
>> +            uint8_t *dst = (uint8_t *)p_video_frame->p_buffer + i_width * i_bpp * y;
>  Useless cast.

Fixed.

>> +        if( p_sys->b_first_frame )
>> +        {
>> +            p_video_frame->i_flags |= BLOCK_FLAG_DISCONTINUITY;
>> +            p_sys->b_first_frame = false;
>> +        }
>  I don't think you need that.

OK.

Do I need to send a signal to reset the PCR, though? I see some comment says
it, but lots of modules don't follow it...

>> +    if( audioFrame )
>> +    {
>> +        const int i_bytes = audioFrame->GetSampleFrameCount() * sizeof(int16_t) * p_sys->i_channels;
>  Using audioFrame->GetBytes() seems simpler.

GetBytes() returns a pointer to the buffer. It does not return the number of
bytes.

>> +        vlc_cond_signal( &p_sys->has_frame );
>> +        vlc_mutex_unlock( &p_sys->frame_lock );
>  You don't need that part. Simply call ES_OUT_SET_PCR and then
> send the data to the es_out here and do not implement the Demux()
> function (set demux_t::pf_demux to NULL). It will be simpler and
> will avoid useless polling.

Hm. Why is dc1394.c written in this way, then? (In general, what kind of
restrictions are there on which threads I can send stuff from?)

How quickly will sending the data return -- can it block? I'm not sure what
demands there are on this callback; I am in the driver's thread, after all.

>> +{
>> +    demux_t     *p_demux = (demux_t*)p_this;
>> +    demux_sys_t *p_sys;
>> +    int         ret = VLC_SUCCESS;
>  I think that setting it to VLC_EGENERIC by default would simplify
> a bit the code.

Done.

>> +    i_card_index = var_InheritInteger( p_demux, "decklink-card-index" );
>> +    for( int i = 0; i <= i_card_index; ++i )
>  Nothing prevent i_card_index from being < 0 leading to result uninitialized.

Added a check.

>> +    psz_display_mode = var_InheritString( p_demux, "decklink-mode" );
> var_CreateGetNonEmptyString() will simplify the following test (== 0 is then not possible).

Done.

>> +    BMDDisplayMode wanted_mode_id;
>> +    memcpy( &wanted_mode_id, &sz_display_mode_padded, sizeof(wanted_mode_id) );
>  Are you sure it is actually big/little endian compatible? (Unless the SDK exists
> only for little endian hardware)

I ntohl() and htonl() as needed, as far as I know. In any case, there's only
support for little endian AFAIK.

>> +    video_fmt.video.i_sar_num = 1;
>> +    video_fmt.video.i_sar_den = 1;
>> +    psz_aspect = var_CreateGetNonEmptyString( p_demux, "decklink-aspect-ratio" );
>> +    if( psz_aspect )
>> +    {
>> +        char *psz_denominator = strchr( psz_aspect, ':' );
>> +        if( psz_denominator )
>> +        {
>> +            *psz_denominator++ = '\0';
>> +            video_fmt.video.i_sar_num = atoi( psz_aspect )      * video_fmt.video.i_height;
>> +            video_fmt.video.i_sar_den = atoi( psz_denominator ) * video_fmt.video.i_width;
>> +        }
>> +        free( psz_aspect );
>> +    }
>  Use var_InheritURational() (for an example, you can look at modules/access/imem.c).

Again, this is copied from other sources, but sure, changed.

>> +    /* Update default_pts to a suitable value for access */
>> +    var_Create( p_demux, "decklink-caching", VLC_VAR_INTEGER | VLC_VAR_DOINHERIT );
>  You don't need to create it. A var_InheritInteger at the right place will
> work.

Same here. Copied, but changed.

New patch attached.

/* Steinar */
-- 
Homepage: http://www.sesse.net/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-access-module-for-BlackMagic-SDI-cards-decklink.patch
Type: text/x-diff
Size: 28056 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20101002/09affea7/attachment.patch>