[vlc-devel] [PATCH 0/1] dshow: Fix assertion when stopping

[Hugo Beauzée-Luyssen]

On Tue, May 30, 2017, at 07:12 PM, Rémi Denis-Courmont wrote:
> Le tiistaina 30. toukokuuta 2017, 18.53.06 EEST Hugo Beauzée-Luyssen a
> écrit :
> > This patch aims at fixing #16935
> > The basic reason for this assertion to trigger is that the access thread
> > enters a single thread appartment.
> 
> VLC does not have an "access thread". The access_demux will, effectively, 

Indeed, I meant "input".

> always run in the input thread - provided that no threaded demuxing
> filters is 
> involved. The access however may well be created in the input thread and
> used 
> in a demux or stream filter thread later.
> 
> In general, a plugin _cannot_ assume that its callbacks are in a single 
> thread.
> 

That seems like even more of a reason to go away from
COINIT_APARTMENTTHREADED. If we can't guarantee the thread from which
things will be called, we either use MTA and deal with synchronisation
if needed, or marshall all of our calls to COM functions. The later
doesn't sound appealing to me.

> > When we stop the playback, the audio
> > output will be flushed, but mmdevice tries to enter a multi thread
> > appartment, which is bound to fail, triggering the assertion.
> > 
> > As things stand, I don't see a reason for dshow to use a STA. All the
> > interfaces I checked don't mention such a requirement, and the only
> > interaction from another thread is FindDevices, which only creates new COM
> > objects.
> 
> Isn´t FindDevices() potentially called from the GUI thread?
> Isn´t the GUI thread entered in an STA?
> 

Only the Qt GUI calls CoInitializeEx (as far as interfaces are
concerned, obviously)
meaning the other interfaces would eventually enter an MTA, should they
invoke FindDevices.
They would also leave the MTA after fetching the devices.

> > As such, it is safe to create & release those objects from another
> > thread without synchronisation.
> 
> As far as I know, it would be safe if and only if the given thread were
> in the 
> MTA when it creates, uses or destroys any of the objects, *and* if the
> MTA has 
> at least one outstanding reference throughout the lifetime of each
> object.

That is the case here. All objects are locally created (within
FindCaptureDevice and AppendAudioEnabledVDevs), and guaranteed to be
destroyed when leaving those function, which is before we call
CoUninitialize

> 
> CommonOpen() meets those conditions. But I don´t see either of those 
> conditions ensured in Demux, Read and both Control functions. Admittedly,
> that 
> is a preexisting bug.
> 
> > That being said, this is my current personal understanding, and I'd gladly
> > stand corrected if I missed something.
> 
> Neither do I know if DirectShow is supposed to work in an MTA. But in any 
> case, the current code and this patch both look incorrect to me.
> 

Given that you say it would be safe if (...), and that this condition is
met, I'm not sure why this patch is wrong. What would be the correct
solution in your opinion?

-- 
  Hugo Beauzée-Luyssen
  hugo at beauzee.fr