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

[Marton Balint]

On Sat, 21 May 2016, Felt, Patrick wrote:

> 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.
>

If you intend to submit that patch as well soon to the mailing list, then 
OK. You can also submit that as a patch series so we can see that keeping 
it is really useful, and no futher changes are necessary in the function.

>>
>>> +{
>>> +    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.

Please, don't mix cleaning up existing code with new features, it makes 
reviewing much harder.

>
>>> +            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

Autodetect what? Sorry, but for me it is still not clear if this means 
supporting the input detection (one card can have multiple inputs), or the 
input format, that is why I would put "input format" somewhere in the name...

>
>>> @@ -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.

Okay, but later, on decklink_start_input you create a whole new decklink 
input callback, shouldn't you release the old? Or am I missing something?

Also are you sure when you start the actual capture, no further 
FormatChange callbacks will fire? So even if the streams are paused, will 
the new EnableVideoInput override the old setting with the format 
detection flag?

That is why I think that maybe it is more simple and reliable to stop 
everything and restart the whole stuff after we acquired the format.

>>> @@ -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.

Okay, but previously on error the code did not list its modes, just 
failed. Also you are returning an invalid error code (AVERROR_EXIT) when 
in fact you should return somehting like AVERROR(ENOSYS) when the 
autodetection is not supported.


>
>>> +
>>> +        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?

If it causes probelms, probably it is not worth the hassle, lets keep it 
as it is then.

>
>>> +            /* 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.

Ok, thanks.

Marton