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

[Jean-Baptiste Kempf]

On Mon, Sep 27, 2010 at 01:56:01PM +0200, Steinar H. Gunderson wrote :
> In any case, I guess this would break the Windows build currently (if the
> user has the SDK installed), so I don't think it's an unreasonable default.
Very unlikely case.

> >> +#ifndef INT64_C
> >> +#define INT64_C(c) c ## LL
> >> +#endif
> > Is this necessary ?
> Yes, it is, given that the module is C++ and the INT64_C stuff is C99.
OK.

> >> +    /* Note: AddRef() and Release() here are not thread safe. */
> > This is an issue, no?
> 
> Actually I don't think so. The only point where they are AddRef()-ed is from
> Open(), and the only point where they are Release()-ed is in Close() (after
> we've shut down the SDI worker threads). Granted, you could say that the SDK
> might AddRef() and Release() the delegate, but BlackMagic's own example code
> actually just has "return 1;" everywhere here, so I don't think it's a big
> problem in practice.
> 
> Does VLC have incremental inc and dec functions I could use?
vlc_gc_incref and such? Not sure if that applied here.

> >> +    int i_card_index = var_CreateGetInteger( p_demux, "sdi-card-index" );
> > Shouldn't you use inherit here?
> 
> Please explain (or point to an example). I have no idea how this stuff works :-)
var_inheritInteger.
Ivoire will comment more.

> strncpy() will add a null byte, which is not what we want. I could always use
> memcpy?
No, discard my comment.

> >> +            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 ?
> 
> I wondered what to do about this one, and it's really easy to miss one or two
> releases in some path. I wonder if the cleanest thing is to put _everything_
> in Close(), even though, say, p_display_iterator is never used outside of
> Open(). What would you prefer -- a goto or putting stuff in Close()?
I don't care. Choose what is better, but don't copy-paste error pathes
in code.

> >> +        msg_Err( p_demux, "Failed to start streams" );
> > What does this mean?
> 
> It means the card refused to start streaming for some reason.
> The documentation lists the following return codes:
> 
>   E_FAIL           Failure
>   S_OK             Success
>   E_INVALIDARG     Is returned on invalid mode or video flags
>   E_ACCESSDENIED   Unable to access the hardware or input stream currently
>                    active
>   E_OUTOFMEMORY    Unable to create a new frame
> 
> Do you want me to reword the message?
Yep.

> Updated patch below.
Formatted git patch, please.

Best Regards,

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