On Sat, 21 May 2016, Felt, Patrick wrote:
On 5/21/16, 3:38 AM, "ffmpeg-devel on behalf of Marton Balint"
<ffmpeg-devel-boun...@ffmpeg.org on behalf of c...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
