On Tue, 10 Jan 2017 12:11:50 +0100
Steve Lhomme <[email protected]> wrote:
> >> + if (SUCCEEDED(hr)) {
> >> + int adapter = atoi(device);
> >
> > Still not liking the atoi() call... (because undefined behavior if
> > device is not actually a number)
>
> If the user/caller gives bogus data it's fair to give bogus results.
It's still bad to silently do something weird if the input is not
well-formatted. Erroring out on broken input is always the preferable
thing to do.
> It's not going to crash though. Also this is the same as DXVA2 so if
> it needs to be changed it needs to be changed there too. Should the
> patchset be delayed until the behaviour is changed in DXVA2 ?
I guess not.
> >> + av_log(ctx, AV_LOG_ERROR, "Failed to create Direct3D device
> >> %lx\n", hr);
> >
> > Just spotted this, but is HRESULT always the C type "long"?
>
> It's always 32 bits.
That doesn't matter. What matters is whether the type is long, because
the "l" specifier expects the type "long", or the behavior is
undefined. I'm fine with it though if we do this incorrectly in most
other Windows code within libav.
> >> +/**
> >> + * This struct is allocated as AVHWDeviceContext.hwctx
> >> + */
> >> +typedef struct AVD3D11VADeviceContext {
> >> + ID3D11VideoDevice *video_device;
> >> + ID3D11VideoContext *video_context;
> >> +
> >> + /**
> >> + * Mutex owned by this context to avoid accessing the video_context
> >> from
> >> + * multiple threads simultaneously.
> >> + */
> >> + HANDLE dev_ctx_mutex;
> >
> > Doesn't say much about those field's lifetime. If you use automatic
> > device creation, I assume freeing the device context frees these
> > interfaces and the mutex.
> >
> > But what if the user supplied these?
>
> It's owned by the context which is allocated internally. It must not
> be set by the caller. There is indeed an issue if an external code
> continues to use the HANDLE after it has been released by the
> hwcontext code.
>
> > Is it possible to prevent it from releasing the mutex?
>
> Maybe it should be created/released by the libavutil caller. We will
> just need to check somewhere that we have an valid handle.
I see no reason why libavutil would create it if the user supplies his
own D3D device/video context. This mutex is needed to protect
concurrent access to them, right? Why would there be possible
concurrent access only if a AVHWDeviceContext is active? What if there
are multiple AVHWDeviceContexts?
Maybe lock/unlock callbacks would be a better choice here after all?
> >> +} AVD3D11VADeviceContext;
> >> +
> >> +/**
> >> + * This struct is allocated as AVHWFramesContext.hwctx
> >> + */
> >> +typedef struct AVD3D11VAFramesContext {
> >> + /**
> >> + * The surface pool. When an external pool is not provided by the
> >> caller,
> >> + * this will be managed (allocated and filled on init, freed on
> >> uninit) by
> >> + * libavutil.
> >> + * When it is provided the allocation/deallocation is up to the
> >> caller.
> >> + */
> >> + ID3D11VideoDecoderOutputView **surfaces;
> >> + int nb_surfaces;
> >> +
> >> + /**
> >> + * Video decoder created by the caller. It must be set before
> >> + * av_hwframe_ctx_init() is called. When decoding is done it will be
> >> + * released.
> >> + */
> >> + ID3D11VideoDecoder *video_decoder;
> >
> > "When decoding is done" isn't very precise. I assume that if the frames
> > context has no more references to it (including ones via AVFrame), it
> > decreases the refcount of video_decoder.
>
> Yes, it means the COM object is released, so the refcounter is
> decreased. Whatever the caller did it may still hold references after
> that or it will be released for good.
I'm just saying it should say that they're unreffed when the frames
context is destroyed, instead of "when decoding is done", which is
vague.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
