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

[Hugo Beauzée-Luyssen]

On Wed, May 31, 2017, at 06:26 PM, Rémi Denis-Courmont wrote:
> Le keskiviikkona 31. toukokuuta 2017, 9.52.26 EEST Hugo Beauzée-Luyssen a 
> écrit :
> > > 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.
> 
> Yes, of course. I didn´t mean to challenge that aspect.
> 
> > 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.
> 
> TBH, I am not familiar with marshalling.
> 
> > > 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)
> 
> Yes but isn´t the Qt Win32 port also entering an apartment internally,
> and isn
> ´t that unfortunately an STA?

It does enter an STA, but it immediately releases it once it's done with
handling file associations. 

> 
> > meaning the other interfaces would eventually enter an
> > MTA, should they invoke FindDevices.
> 
> FindDevices() has to release whichever apartment that it succesfully
> entered. 
> Otherwise we would have no way to prevent leakage.
> 

Which it always does (provided CoInitializeEx succeeds)

> > 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
> 
> The enumeration callbacks indeed seem fine. But what about the access 
> callbacks? What happens if Open() and Read(), or Open() and Close() are 
> serialized yet called in different threads?
> 
> (...)
> > 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?
> 
> AFAICT, either the DirectShow code has a thread keeping the MTA alive for
> the 
> lifetime of the access/access_demux instance, or we ensure that another
> thread 
> takes care of the same (e.g. in Win32 specific core code).
> 

The MTA gets released by the dshow module from its Close() function, but
if I understand you correctly, you're worry is that Open and Close won't
be called from the same thread, right?
To be honest I'd have assumed it was always the case

> MMdevice has a background thread taking care of that, so the problem
> might 
> only be apparent if you disable audio. The SAPI plugin is also buggy in
> this 
> respect, IIRC.
> 

I didn't reproduce the issue with audio disabled with this patch, but
granted, that's no proof.

Regards,

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