cplussharp / graph-studio-next

GraphStudioNext is a tool for developers to build and test DirectShow Graphs
355 stars 94 forks source link

Graph clock not being set correctly #291

Closed EnigmaIndustries closed 7 years ago

EnigmaIndustries commented 8 years ago

Check the "Use Clock" menu item, create a graph that includes the Default DirectSound Device like this:

clock

Default clock selecting behavior should choose the clock on the Default DirectSound Device, but it is not. I checked the pointer returned by the graph managers IMediaFilter::GetSyncSource and the pointer returned by the Default DirectSound Device IMediaFilter::GetSyncSource and they do not match. I'm not sure when the graph manager runs it's default clock selecting procedure to choose a clock, perhaps it is first time the graph managers IMediaFilter::GetSyncSource is called? Which in GraphStudioNext is before any filters have been added to the graph. Perhaps a call to IFilterGraph::SetDefaultSyncSource before the graph is run might fix it.

cplussharp commented 7 years ago

I have extended the status bar to show the current sync source in commit 92106f5d5fd2c2719cab3d5e18c74ed757317436. You are right, the system clock is used and not the audio renderer clock. For every new Graph SetDefaultSyncSource is called shortly after the creation. So this algorithm is not executed automatically on the first run of the graph. I'm not sure why it is done this way, but if I change it, there is no other way to test a filter graph with audio with the "System Reference Clock". I don't know what to do right now and I hope the new status bar text is a step to more awareness about this behavior.

mikecopperwhite commented 7 years ago

Direct Show doesn't set a reference clock during graph building even in GraphEdit, it relies on determining the best clock when streaming starts. We could automatically set a preferred reference clock in GSN but from the MSDN docs it's not DirectShow best practice to do so.

According to the documentation for Reference Clocks from MSDN: https://msdn.microsoft.com/en-us/library/windows/desktop/dd377506(v=vs.85).aspx

====================

An application can select a clock by calling the IMediaFilter::SetSyncSource method on the Filter Graph Manager. You should do this only if you have a particular reason to prefer another clock.

Default Reference Clock

====================

Our clock display indicates the reference clock explicitly selected by the application (an override clock in effect).

AFAICT We can't find out which actual reference clock is selected by DirectShow when playback starts with a default reference clock. Intercepting the filter graph manager calling IMediaFilter::SetSyncSource on a filter only detects when GSN sets a reference clock manually, it doesn't get called when streaming starts with a default clock selected (tested using the Analzyer Filter implementation of SetSyncSource).

One change we could make - if a filter that has been manually set as the reference clock is unset by clicking on the clock icon again, revert to the default reference clock rather than turn the clock off entirely. Not quite the GraphEdit behaviour but more useful as we can then we get flip between default clock and explicitly clock directly. There's already a separate UI feature for turning clock on and off.

BTW the clock display feature on the status bar is a nice addition!

mikecopperwhite commented 7 years ago

Have tried resetting the default reference clock using SetDefaultSyncSource but it doesn't reset to the System reference clock in all cases. If SetSyncSource has been called on an audio renderer filter in the graph, calling SetDefaultSyncSource always sets the reference clock back to the audio renderer filter. Turning clock on and off doesn't change this. The audio renderer clock seems to be 'sticky' in some way - perhaps related to the symptoms in the initial report.

Not sure there's anything else to do on this issue.

cplussharp commented 7 years ago

I think that is the correct behavior as the Audio-Renderer is higher ranked in the algorithm the MSDN describes. If the application has set no clock, then SetDefaultSyncSource selects the clock of a live source or the clock of a filter, beginning with the renderer.

Maybe we need an option, to not call SetDefaultSyncSource on an empty filter graph? And this option should always be off, so it's more like the default DirectShow behavior?

mikecopperwhite commented 7 years ago

Option to turn on/off SetDefaultSyncSource sounds good or perhaps more flexibly a simple menu item to do a one off call to SetDefaultSyncSource when the user chooses. Hard to tell if GraphEdit is doing this as we don't have any filters in a graph at startup that we could debug.

Note that it's possible to end up with 'no clock' if you set the audio renderer as the clock then remove that filter from the graph. The clock doesn't get set back to System Reference Clock.

Interestingly 'no clock' is the same result as creating a graph without calling SetDefaultSyncSource at graph creation suggesting 'no clock' is the correct starting state from which DirectShow would automatically select the correct clock.

mikecopperwhite commented 7 years ago

Ah, in the 'no clock' state starting playback selects the audio renderer as the clock but the status bar isn't updated to show this until the graph is edited (e.g. another filter added). Sounds like 'no clock' is definitely the correct starting state and the automatic clock selection will be visible in the status bar.

Not sure at which point we should update the clock display - when streaming is started or stopped. Update on start would be useful unless clock selection can happen later than this.

mikecopperwhite commented 7 years ago

As far as I can tell the clock setting behaviour now matches GraphEdit. If the user creates a new graph and doesn't change the clock settings, DirectShow will choose a clock when playback starts and this is reflected in the status bar.

The user can get the previous behaviour (not recommended by MSDN docs) by creating a new graph then choosing 'use clock' which will call SetDefaultSyncSource.

Feel free to reopen this issue if there are remaining problems.