GNOME Bugzilla – Bug 689191
d3dvideosink improvements
Last modified: 2016-04-19 07:46:58 UTC
The attached patch provides the following enhancements: * XOverlay set_render_rectangle support (Useful for rendering in QT QML) * RGB render support * Video format negotiation (with preference ordering) * Using Direct3D9 (No benefit to using newer D3D versions) It looks like there have been some updates since the snapshot I worked off of, but I think still compelling with render_rectangle and RBG support. There was quite a big code re-organization in this patch.
Created attachment 230052 [details] [review] d3dvideosink patch
Could you update the patch to apply cleanly against git master? Also separate patches for the separate changes would be useful and make reviews easier.
Created attachment 231588 [details] [review] d3dvideosink patch for gstreamer master I've updated the previous 0.10 branch patch for master (1.1)
Making separate patches is not possible at this point. I originally cleaned up the code by removing duplicate code and creating gst/d3d separation before working on the new features. The new master patch has a few improvements over the 0.10 patch.
That's bad, as is this patch is mostly unreviewable because it basically changes everything everywhere :(
Spent some time reviewing this, I hope I didn't miss anything... commit fe5f514049029fd9004074c9a3970e27b8bc5120 Author: Roland Krikava <rkrikava@gmail.com> Date: Sat Dec 22 11:24:28 2012 +0100 d3dvideosink: Various improvements * XOverlay set_render_rectangle support (Useful for rendering in QT QML) * Video format negotiation (with preference ordering) * Using Direct3D9 (No benefit to using newer D3D versions)
I should add that there are quite some things that need improvement but I'm working on that now
This removed work done to abstract the Direct3D version used but more importantly, it removed work done to allow the clean selection of alternative video sinks (such as the DirectShow or DirectDraw video sinks) should Direct3D either not be installed or working properly (which is quite possible -- e.g. on Windows XP embedded devices -- and the impetus for putting that logic in there in the first place). I'm sure the motivation was to remove unused code and simplify things -- but it went beyond that and it was removed without insight into its original intent. Please revert those changes or reintroduce them.
(In reply to comment #8) > This removed work done to abstract the Direct3D version used but more > importantly, it removed work done to allow the clean selection of alternative > video sinks (such as the DirectShow or DirectDraw video sinks) should Direct3D > either not be installed or working properly (which is quite possible -- e.g. on > Windows XP embedded devices -- and the impetus for putting that logic in there > in the first place). What exactly do you mean? It should fail the state change to READY (doesn't it?) and then a different sink would be auto-selected.
Hi, David. Yes, I removed the abstractions to newer versions of D3D because the newer versions are backward compatible and offer interfaces to D3D9.
Sebastian: it can fail a state change to READY if Direct3D is explicitly referenced but not installed on the system. IOW, it would fail at library load time. The previous code effectively used LoadLibrary() (well, the glib equivalents) to detect Direct3D's existence and the correct version at that before attempting to use it. There was logic in place that could be used in the future to select a better version of Direct3D if available and then fail back to v9 if none were found. Windows XP embedded (for example) allows you to pick and choose which components of the Windows runtime you want on the system, so it's entirely feasible that Direct3D may not exist where others might (e.g. DirectDraw). If you bring up the old d3dvideosink dll in dependency walker, you'd notice that there's no explicit reference to the Direct3D 9 libraries -- they're detected and loaded at runtime to explicitly enable a graceful failover to alternative technologies. It's feasible that Direct3D 9 will be dropped in the future (as in shipped OOTB) and it's also possible that video card manufacturers in the future could drop support for Direct3D 9. Admittedly, in the foreseeable future it's unlikely at best. Roland -- yes DX10/11 are backwards compatible but that could change. We're talking about Microsoft here. The DX10/11 code, if you recall, wasn't even being used (really for that explicit reason), but if the next version does break compatibility, you'd have to reintroduce something to abstract the version away since older clients (e.g. WinXP) would be unlikely to have support for it. Sebastian -- this should have been reviewed more in-depth prior to being committed. Some parts are absolutely fine, others not (e.g. the topic of this discussion). Shouldn't this have been split into multiple patches?
Sebastian: While I'm at it, and I apologize for hijacking this thread, I saw another commit 'remove scary "while (object.refcount > 0) release (object);"' -- I agree that's scary but the reason for that being in there is due to the fact that video card manufacturers actually implement part of the DirectX API and some of their implementations are just plain buggy and they forget to unreference causing a leak. It's not for covering up a logic error. In an ideal world, I agree it's a bad idea. Perhaps it would be better to fail fast instead of ignoring the problem and allowing the memory leak. When the sink was first written I had to use it on systems that ran 24/7 for years and so slow memory leaks over time become huge problems. The original code was tried on several video cards over a few different vendors. ATI cards were typically the worst offenders IIRC. And that hack eliminated the leaks. Just my $0.02...
(In reply to comment #11) > Sebastian: it can fail a state change to READY if Direct3D is explicitly > referenced but not installed on the system. IOW, it would fail at library load > time. The previous code effectively used LoadLibrary() (well, the glib > equivalents) to detect Direct3D's existence and the correct version at that > before attempting to use it. There was logic in place that could be used in the > future to select a better version of Direct3D if available and then fail back > to v9 if none were found. > > Windows XP embedded (for example) allows you to pick and choose which > components of the Windows runtime you want on the system, so it's entirely > feasible that Direct3D may not exist where others might (e.g. DirectDraw). Yeah, but then playbin will automatically switch to directdrawsink. It doesn't matter much if the failure happens at plugin load time or during the state change to READY. It only makes everything more complex IMHO :) > It's feasible that Direct3D 9 will be dropped in the future (as in shipped > OOTB) and it's also possible that video card manufacturers in the future could > drop support for Direct3D 9. Admittedly, in the foreseeable future it's > unlikely at best. Direct3D 9 is a requirement for Metro compatible devices at this point. As such I don't see it getting dropped in the next years at all, especially considering that Microsoft is annoyingly concerned with backwards compatibility. > Sebastian -- this should have been reviewed more in-depth prior to being > committed. Some parts are absolutely fine, others not (e.g. the topic of this > discussion). Shouldn't this have been split into multiple patches? Yes, it should've been split up into multiple patches, see discussion at the very top here. I tried to review it as good as possible though and apparently missed some pieces, sorry for that. So, let's reopen this bug and make sure it gets fixed before the 1.2 release.
(In reply to comment #12) > Sebastian: While I'm at it, and I apologize for hijacking this thread, I saw > another commit 'remove scary "while (object.refcount > 0) release (object);"' > -- I agree that's scary but the reason for that being in there is due to the > fact that video card manufacturers actually implement part of the DirectX API > and some of their implementations are just plain buggy and they forget to > unreference causing a leak. It's not for covering up a logic error. > > In an ideal world, I agree it's a bad idea. Perhaps it would be better to fail > fast instead of ignoring the problem and allowing the memory leak. > > When the sink was first written I had to use it on systems that ran 24/7 for > years and so slow memory leaks over time become huge problems. > > The original code was tried on several video cards over a few different > vendors. ATI cards were typically the worst offenders IIRC. And that hack > eliminated the leaks. Just my $0.02... The problem with this hack is that it's completely not threadsafe and fragile :) If it was used to work around bugs in drivers, first of all this should be reported to the vendor providing this driver. The leak won't be only happening in GStreamer but also in other applications. I would be fine with re-introducing it, but only actually use that hack if an environment variable is set.
(In reply to comment #13) > Yeah, but then playbin will automatically switch to directdrawsink. It doesn't > matter much if the failure happens at plugin load time or during the state > change to READY. It only makes everything more complex IMHO :) playbin deals with system loader issues? What I'm talking about are OS-level undefined/unknown reference errors -- the dll itself (at least not typically) won't get past the OS loader into user space if its dependencies aren't satisfied. The code from before looks (to the system loader) like there aren't any Direct3D dependencies. > Direct3D 9 is a requirement for Metro compatible devices at this point. As such > I don't see it getting dropped in the next years at all, especially considering > that Microsoft is annoyingly concerned with backwards compatibility. I still give my Windows XP embedded example, which I know is actually still in use. Plus effort has already been done to abstract Direct3D -- why eliminate it (especially when it is providing a useful function of gracefully downgrading when Direct3D is unavailable on the system)? > So, let's reopen this bug and make sure it gets fixed before the 1.2 release. Thanks!
(In reply to comment #15) > (In reply to comment #13) > > Yeah, but then playbin will automatically switch to directdrawsink. It doesn't > > matter much if the failure happens at plugin load time or during the state > > change to READY. It only makes everything more complex IMHO :) > > playbin deals with system loader issues? What I'm talking about are OS-level > undefined/unknown reference errors -- the dll itself (at least not typically) > won't get past the OS loader into user space if its dependencies aren't > satisfied. The code from before looks (to the system loader) like there aren't > any Direct3D dependencies. Ah now I finally understand what you mean. Well, if the plugin fails to load because of missing references it will be ignored by the registry. No problem there at all then :) > > Direct3D 9 is a requirement for Metro compatible devices at this point. As such > > I don't see it getting dropped in the next years at all, especially considering > > that Microsoft is annoyingly concerned with backwards compatibility. > > I still give my Windows XP embedded example, which I know is actually still in > use. But Windows XP has no Direct3D at all, right? In that case the Direct3D plugin wouldn't be used and directdrawsink (for example) would be used as fallback. > Plus effort has already been done to abstract Direct3D -- why eliminate it > (especially when it is providing a useful function of gracefully downgrading > when Direct3D is unavailable on the system)? Mostly because of the (from my understanding) unneeded code complexity (and thus maintenance overhead), and complexity during the build. Now this just uses the system Direct3D headers and properly links to everything (thus failing properly during link time, and during library load time).
(In reply to comment #16) > Ah now I finally understand what you mean. Well, if the plugin fails to load > because of missing references it will be ignored by the registry. No problem > there at all then :) Still have the added benefit of doing runtime analysis of the best version to load/use. :D IIRC even though later versions are backwards compatible, that compatibility comes at a slight cost -- that is, certain operations that would be more performant with later versions require extra work to be compatible w/ v9 causing a hit (likely small). But it's been a long time since I read up on that so my memory could be off. I should also add that later versions do diverge in several ways in their approach and even the solution I had provided might require further tweaking -- but at least it's a start. :D > Mostly because of the (from my understanding) unneeded code complexity (and > thus maintenance overhead), and complexity during the build. Now this just uses > the system Direct3D headers and properly links to everything (thus failing > properly during link time, and during library load time). It really wasn't that complex. The old code used the system Direct3D headers and would fail to compile if they weren't present. IIRC, I didn't implement the DX10/11 b/c they were backwards compatible w/ 9 but also b/c mingw-w64 didn't have good (or any), usable DX10/11 headers.
(In reply to comment #16) > But Windows XP has no Direct3D at all, right? In that case the Direct3D plugin > wouldn't be used and directdrawsink (for example) would be used as fallback. No, I believe it does.
To be honest I'm not quiet following the discussion. First point I'd like to make off the bat, is that according to Microsoft Direct3D9 is supported as of Windows XP through to Windows 8: Use Direct3D 9 APIs with Windows XP and later. All hardware supports Direct3D 9 APIs, even newer Direct3D 11-level hardware. From following link, under 'Which Direct3D version?': http://msdn.microsoft.com/en-us/library/windows/desktop/hh769064(v=vs.85).aspx If for some reason a system does not have d3d9.dll, as Sebastian has mentioned the plugin would fail to load, and another display element would be used. What am I missing?
Anything missing here? The comments are not really conclusive :)
No movement for 2 years, assuming all is done and closing.