On 5/21/16, 3:38 AM, "ffmpeg-devel on behalf of Marton Balint"
<[email protected] on behalf of [email protected]> 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.
_______________________________________________
ffmpeg-devel mailing list
[email protected]
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
