[MPlayer-dev-eng] [PATCH] DeckLink video output

[Reimar Döffinger]

On Mon, Aug 17, 2009 at 10:22:58PM +0200, Georg Lippitsch wrote:
> The patch works well for me, but it is C++ and has some minor issues, so I'm 
> not sure if it is acceptable. Any comments are very welcome, of course!

Well, that it has to use C++ while using only stuff that can be done
just fine in plain C sure is annoying.

> +static int in_rate = 0;
> +static int in_channels = 0;
> +static bool running = false;
> +static uint64_t total_samples = 0;

pointless initialization.

> +    if (format != AF_FORMAT_S16_LE)
> +        return 0;

Just set ao_data.format to whatever you need to get automatic
conversion.

> +    HRESULT hr = decklink_out->EnableAudioOutput(rate,
> +                                                 bmdAudioSampleType16bitInteger,
> +                                                 channels,
> +                                                 bmdAudioOutputStreamContinuous);
> +    
> +    if (hr == S_OK)
> +    {
> +        in_rate = rate;
> +        in_channels = channels;
> +
> +        return 1;
> +    }
> +
> +    return 0;

return hr == S_OK;
because you already have rate and channels in ao_data, no need for
another copy.

> +static void uninit(int immed)
> +{
> +}

Disregarding that the design without open/close or anything seems really
horrible to me (does this thing always allocate resources just when
MPlayer is linked against it, regardless of whether it will ever be used
or not??) this is also wrong, when immed is set it should stop playing
immediately.
I guess calling reset() might do.

> +static void reset(void)
> +{
> +    running = false;
> +    total_samples = 0;
> +    decklink_out->FlushBufferedAudioSamples();
> +}
> +
> +static void audio_pause(void)
> +{
> +    reset();
> +}

Is there no way to stop output without dropping the buffered samples?

> +static int get_space(void)
> +{
> +    return 38400;
> +}
> +
> +static float get_delay(void)
> +{
> +    return 0.0;
> +}

At least in some cases this combination is going to cause severe
issues. E.g. with audio only files it might cause an extremely high CPU
usages (e.g. 40% instead of 0.5%).
Unfortunately I do not remember the details.
There is also the issue that A-V sync will not be remotely good without
an accurate get_delay.

> +    if (samples_in + total_samples > samples_max)
> +    {
> +        samplesToWrite = samples_max - total_samples;
> +    }
> +    else
> +    {
> +        samplesToWrite = samples_in;
> +    }

You mean
samplesToWrite = FFMIN(samples_in, samples_max - total_samples);
?