[FFmpeg-devel] [PATCH] Updated (v3) -- Add input mode autodetect to the decklink module.

[Felt, Patrick]

On 5/21/16, 3:38 AM, "ffmpeg-devel on behalf of Marton Balint" <ffmpeg-devel-bounces at ffmpeg.org on behalf of cus at passwd.hu> wrote:

>> --- a/libavdevice/decklink_common.cpp
>> +++ b/libavdevice/decklink_common.cpp
>> @@ -98,6 +98,90 @@ HRESULT ff_decklink_get_display_name(IDeckLink *This, const char **displayName)
>>     return hr;
>> }
>>
>> +long ff_decklink_mode_to_bm(AVFormatContext *avctx,
>
>Should be BMDDisplayMode, not long.
>
>> +                               decklink_direction_t direction,
>> +                               int ffmpeg_mode,
>> +                               IDeckLinkDisplayMode **mode)
>
>As far a I see you do not use **mode with a non-NULL arugment in your 
>code, so you can get rid of it and its functionality.

True, in this patch I do not use **mode, however I noticed that elsewhere we did a similar loop.  This could consolidate the code into one fuction so we don’t have duplicate code.  That would definitely be an unrelated change so I left it open enough to hopefully suffice.  We can take it out if we need to.
 
>
>> +{
>> +    struct decklink_cctx *cctx = (struct decklink_cctx *) avctx->priv_data;
>
>unnecessary space before avctx

most of the spaces here are because I copied and pasted those lines from other, previously defined functions.  I removed from where I was seeing them, however I may have removed some extras.  Please don’t consider those a formatting change.

>> +            break;
>> +        }
>> +
>> +        internal_mode->Release();
>> +    }
>> +
>> +    itermode->Release();
>> +    if (internal_mode) {
>
>What if there is no match for ffmpeg_mode? Is it documented in the 
>Decklink SDK that internal_mode will be overwritten to NULL on the last 
>iteration?

Good catch.  I’ll rework this loop.

>> +int ff_decklink_mode_to_ffmpeg(AVFormatContext *avctx,
>> +                               decklink_direction_t direction,
>> +                               IDeckLinkDisplayMode **mode)
>
>should use *mode, not **mode, because *mode is not overwritten in this 
>function

modified this one as there really isn’t a need to send back mode information

>> +int ff_decklink_device_autodetect(AVFormatContext *avctx)
>
>Probably worth remaining to somehting like 
>ff_decklink_can_detect_input_format otherwise somebody may be 
>under the impression that this function will do the autodetection.

I’ve modified this to ff_decklink_device_supports_autodetect

>> @@ -244,6 +245,12 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>>     BMDTimeValue frameTime;
>>     BMDTimeValue frameDuration;
>>
>> +    /* if we don't have video, we are in the autodetect phase.  skip everything and let
>> +     * autodetect do its magic */
>> +    if (!ctx->video) {
>> +        return S_OK;
>> +    }
>> +
>>     ctx->frameCount++;
>>
>>     // Handle Video Frame
>> @@ -393,6 +400,14 @@ HRESULT decklink_input_callback::VideoInputFormatChanged(
>>     BMDVideoInputFormatChangedEvents events, IDeckLinkDisplayMode *mode,
>>     BMDDetectedVideoInputFormatFlags)
>> {
>> +
>> +    /* undo all the autodetect stuff so we can move on with life */
>> +    ctx->dli->PauseStreams();
>> +    ctx->dli->FlushStreams();
>> +
>> +    ctx->mode_num = ff_decklink_mode_to_ffmpeg(avctx, DIRECTION_IN, &mode);
>> +    ctx->video = 1;
>
>I would only do anything in this function, if ctx->auto_detect is set 
>to 1, and I would also set ctx->auto_detect to 0, so you don't have to 
>use a separate ->video variable for signalling a successful autodetection.
>
>Also don't you want to StopStreams and DisableAudio/VideoInput here? 
>Because you will be re-doing the whole initialization stuff later, and I 
>am not sure you are supposed to call ff_decklink_set_format when the 
>streams are already running.
>

The decklink sdk specifically states that there should be a pause here and not a stop/start.  Also, ff_decklink_set_format() only checks that a mode is supported.  It doesn’t actually do anything else with the decklink api.

>> @@ -510,34 +525,74 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx)
>>
>>     /* Get input device. */
>>     if (ctx->dl->QueryInterface(IID_IDeckLinkInput, (void **) &ctx->dli) != S_OK) {
>> -        av_log(avctx, AV_LOG_ERROR, "Could not open output device from '%s'\n",
>> +        av_log(avctx, AV_LOG_ERROR, "Could not open input device from '%s'\n",
>>                avctx->filename);
>>         ctx->dl->Release();
>>         return AVERROR(EIO);
>>     }
>>
>> +    auto_detect = ff_decklink_device_autodetect(avctx);
>> +
>>     /* List supported formats. */
>> -    if (ctx->list_formats) {
>> +    if (ctx->list_formats || (ctx->mode_num <= 0 && !auto_detect)) {
>
>This seems like an unrelated change, i'd drop it for now.

If the user specifies they want auto detection, and their card doesn’t support it, I display the supported modes and exit.  This is related.

>> +
>> +        result = ctx->dli->EnableAudioInput(bmdAudioSampleRate48kHz, bmdAudioSampleType16bitInteger, cctx->audio_channels);
>> +        if (result != S_OK) {
>> +            av_log(avctx, AV_LOG_ERROR, "Could not enable audio in the pre-startup autodetect mode\n");
>> +            goto error;
>> +        }
>
>Are you sure you need audio? Maybe for auto detection, audio is not that 
>important, also what if the first format does not have that many audio 
>channels, as many the user wants...

That’s an excellent question, and I originally tried to enable only video and not audio during auto detection, however if I didn’t enable audio at the same time as video the two got out of sync.  I had to enable both at the same time even with a full stop/start.  I suppose ideally we could check stream_specs for if the user wants audio.  I can add that here or in a different patch?

>> +            /* sleep for 1 second */
>> +            av_usleep(100000);
>
>That's actually 0.1 sec.

heh.  Good catch on that one.  I’ll fix it.

>
>> +        }
>
>You should rework this loop a bit. For starters, probably it is better if 
>the user can set the maximum autodetection delay using milisecond 
>precision. On the other hand, the sleep time between your checks if the 
>autodetecton is completed can be a constant 0.01 second.
>
>Also, as I mentioned earlier I suggest you don't use the ->video variable, 
>only the ->auto_detect, so in order to detect a successful detection you 
>will wait in the loop until ->auto_detect becomes 0.

I’ll think through this one and see if I can’t come up with something else when I repost.

>> @@ -618,6 +673,7 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx)
>>
>>     return 0;
>>
>> +
>
>Unrelated change

I’m assuming you mean the empty line addition?  I can remove that.  Most of the rest of the changes were pretty simple and done.  I’ll resubmit after we get the last bit of discussion figured out.