Created attachment 194231 [details] [review]
don't share one D3D device, instead create one for each instance

I'm not sure how waiting for a vsync to issue d3d commands will avoid tearing except by getting lucky.  It would be better to use explicit synchronization operations if possible.

Also, there is a bunch of extraneous changes in the 194229 patch.  Part of this would have been cleaned up if you used gst-indent, but presumably you don't have that available on Windows.  If such is the case, please clean up as possible and I'll reindent while reviewing.

My assumption that the GPU flushing works is the asynchronous nature of the D3D stretch and color space conversion operation. The Present call doesn't seem to wait for a completion. What happens then is while the operation is still in progress (GPU) the back buffer will be filled with the next frame. This buffer fills show up on the screen. So waiting for the async operation to complete should solve this.  
And my test have shown that this improves tearing a lot (testet on NVIDA and Intel GFX).

Sorry for the bad indentation, indeed I don't have gst-indent on my Windows machine. I will try to clean up the patch.

Created attachment 194376 [details] [review]
flush GPU before Present

Added missing Release and gst-indent formatted.

Created attachment 194377 [details] [review]
don't share one D3D device, instead create one for each instance

some cleanup and gst-indent formatted.

commit 0c5037072ad18394188960a6c4d346f22609a864
Author: Carsten Kroll <car@ximidi.com>
Date:   Mon Aug 22 16:31:38 2011 +0200

    d3dvideosink: create a d3d device for each sink and use the default swap chain in connection with flushing the GPU and not recreating the swap chain this avoids tearing also use GST_xx_DEBUG macros where appropriate

commit ab22a64a1331dafa9c6ca542845240ada6850b48
Author: Carsten Kroll <car@ximidi.com>
Date:   Mon Aug 22 14:46:48 2011 +0200

    d3dvideosink: flush GPU before Present added alternate wait for vsync method based on GetRasterStatus

Reopening, as 0c5037072ad18394188960a6c4d346f22609a864 causes regressions as noted in #670143.

*** Bug 670143 has been marked as a duplicate of this bug. ***

Carsten: how did this still compile for you? shared.d3ddev was removed, but still referenced in the code.

commit 740c3101ac538a4a3f7e502aa2d8ceea54c2dca6
Author: Руслан Ижбулатов <lrn1986@gmail.com>
Date:   Fri Mar 2 21:41:39 2012 +0400

    d3dvideosink: fix compiler warnings and build failure with mingw
    
    shared.d3ddev was removed a while back, not sure how this still
    compiles for anyone (tpm).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=653718
    https://bugzilla.gnome.org/show_bug.cgi?id=670143
    https://bugzilla.gnome.org/show_bug.cgi?id=656905

In my code there is no reference to shared.d3ddev anymore. That should have been removed with patch #2. I'm also using the Microsoft compiler by the way. But it shouldn't matter.

patch #194377 I was referring to.

Could you attach a diff between the current version in -bad and your version (i.e. what it should really look like)? I guess something went wrong when git automatically merged the patch.

Commit d3b1488fa734b4aa025845feb0590bf46d0983f5 dated August 27 adds a check for shared.d3ddev. Commit 0c5037072ad18394188960a6c4d346f22609a864 dated August 22 removes shared.d3ddev -> code doesn't compile. Commit 740c3101ac538a4a3f7e502aa2d8ceea54c2dca6 removes the check -> code compiles again.

But as mentioned in bug 670143, after 0c5037072ad18394188960a6c4d346f22609a864 I get a black screen and this error:

d3dvideosink d3dvideosink.c:1908:gst_d3dvideosink_stretch:<d3dvideosink1> StretchRect failed

I tested this again and the error only seems to occur with some pipelines (no idea why). With an MPEG-2 transport stream file, this works:

gst-launch filesrc location=/test.trp ! decodebin2 ! d3dvideosink

But this doesn't:

gst-launch playbin2 uri=file:///test.trp video-sink=d3dvideosink

Same problem here: mpeg2 = black screen, h264 = works

And it's not possible to switch the output window via gst_x_overlay_set_window_handle() on running pipeline.

The problem might be related to the negotiated colorspace. I420 vs. NV12 ... just a guess.

I attached the diff between my version and the latest git. But they are almost the same. My version has the old "xwindow_id" names because it was linked against an older base. And of course the previously mention shared.d3ddev is missing.

Created attachment 209118 [details]
diff to my sources.

> The problem might be related to the negotiated colorspace.
When I don't set the internal window of my gui via gst_x_overlay_set_window_handle() and let d3dsink create his own window on start then it works here for me. So the problem may be related to the above mentioned problem that the window can also not be switched on running pipeline.

Created attachment 209895 [details] [review]
regression fix

this patch should solve the problem with calling gst_x_overlay_set_window_handle() in pause/play state

Can this be closed again now, or what's left that needs fixing?

Switching the window works now for me, but the problem with black screen (see comments 13+14) still needs to be fixed.

Is there a reason that it was changed to 1 device per sink? It's my understanding that that can result in significant performance penalties. It's typically better to create multiple swap chains instead. Is there something I'm missing? Please educate me. :D

Review of attachment 194377 [details] [review]:

The recommended approach for performance reasons is typically to have 1 device and additional swap chains. The tearing issue is odd -- especially in Vista+. I haven't noticed tearing with the previous approach running multiple simultaneous videos in the same application. Instead, you might want to inspect your graphics driver and look for an update from your manufacturer. The previous code was tested on multiple cards from multiple manufacturers.

Review of attachment 194377 [details] [review]:

I feel this should be reverted and it seems there's a bug in the device lost logic. I admit my memory's a bit fuzzy -- won't you have to recreate every device on a device loss? What I see here seems to indicate there could be a memory leak as old d3d devices aren't correctly released after a device loss.

::: sys/d3dvideosink/d3dvideosink.c
@@ +2050,3 @@
   /* Ask DirectX to please not clobber the FPU state when making DirectX API calls. */
   /* This can cause libraries such as cairo to misbehave in certain scenarios. */
+  d3dcreate = 0 | D3DCREATE_FPU_PRESERVE | D3DCREATE_MULTITHREADED;

D3DCREATE_MULTITHREADED should not be needed. Device access should always be taking place on the rendering thread. Messages are posted to the render thread otherwise. Access is already guarded and D3DCREATE_MULTITHREADED is therefore redundant and only a performance hit.

@@ +2137,2 @@
+  ZeroMemory (&sink->d3dpp, sizeof (sink->d3dpp));
+  sink->d3dpp.Flags = D3DPRESENTFLAG_VIDEO | D3DPRESENTFLAG_LOCKABLE_BACKBUFFER;

Is this needed? This can incur add'l performance costs. See http://msdn.microsoft.com/en-us/library/windows/desktop/bb172586(v=vs.85).aspx

The approach with multiple devices worked best in a multi monitor setup.
The tearing with the previous solution was very bad in this situation. Especially when turning of Aero in Vista/Win7 and on XP.
You are right that the device lost logic should probably revised. 
On the cards I tested this solution there seems to be no performance problems. 
A simple pipeline I used to test the tearing is: 
gst-launch-0.10.exe videotestsrc pattern=bar horizontal-speed=15 ! video/x-raw-yuv,framerate=25/1 ! d3dvideosink

The cards I tested are Nvidia GTX280, Intel HD Graphics on Core i5 and a Radeon HD ????.

My own tests seemed to indicate otherwise. Mine ran on multiple monitors as well (typically 2-4) in XP, Vista, and 7 and across cards from the same manufacturers (it's been too long for me to remember the exact models). Did you notice tearing on all machines with multiple monitors using the old code? Or did you notice tearing on one, make your changes, and then test on the others afterward?

My concern is that this is a step backward as the decision to use multiple swap chains instead of multiple devices was a conscious, weighed decision and a lot of effort was put into making it work right.

I don't know of anyone else who has reported this issue and I know there's more using it than you and I and so your setup/configuration seems more suspect IMO.

If multiple devices is truly necessary, I still feel this patch needs to be revisited and revised and address some of the things I've noted. We shouldn't be committing things known to cause memory leaks/issues.

As far as can remember I noticed the tearing more or less on all kind of setups, Multiple cards/monitors or even on single cards/monitors. Since this was not acceptable I tried several things to solve this, but I had no success using the multiple swap chain approach. So I tried it with the multiple device solution. 
And it worked for my use case.

It is very easy to see the difference with the simple pipeline I mentioned earlier.

I don't doubt that a lot of effort was put into this. So if you feel this patch is wrong then it's of course up to you to reject it. In the end it's been just a suggestion.

Created attachment 215270 [details] [review]
Create the back buffer with the correct size

The back buffer was created without taking the pixel aspect ratio in account, creating a smaller surface for pixel-aspect-ration > 1 and therefore making StretchRect fail trying to scale into a smaller rectangle.
This bug was only exposed in playbin2, which sets pixel-aspect-ratio=true.

Created attachment 215271 [details] [review]
Don't flush the gpu after a device lost.

Flushing the GPU is called twice and the last one might happen after a device lost, when the device might already been destroyed.

Is everything fixed here now?

I tested 0.10 branch and video playback works now, so the black screen problem seems to have been fixed. But if I minimize the video window in Windows 7, I still get a lot of these error messages:

d3dvideosink d3dvideosink.c:1916:gst_d3dvideosink_stretch:<d3dvideosink0> StretchRect failed

Video playback continues normally after restoring the window, so it's not a big problem.

I also have some problem with video quality, video looks much smoother in 0.10.22. The problem is especially visible when rendering DVB subtitles. I'll try to investigate this.

Do these issues still happen in 1.x ?

Closing this bug report as no further information has been provided. Please feel free to reopen this bug report if you can provide the information that was asked for in a previous comment.
Thanks!