GNOME Bugzilla – Bug 793722
Capture screenshot of single window fails if the window is ie 11
Last modified: 2018-05-15 23:31:00 UTC
Created attachment 368770 [details] Gimp screenshot vs SnagIt screenshot (what it actually looks like) Brand new computer (Windows 10), installed GIMP 2.8.22. Trying to capture screenshots of our apps which display best in IE (I know, I know, our users are old people). File > Create > Screenshot > Grab a single window > Grab > Move + over IE 11.248.16299.0 window Screencap completes but shows incorrectly - content of page is mostly missing, format of URL area is not what shows on screen. Attachment shows same screen captured in GIMP vs. in Snagit. Note: I had same problem in GIMP 2.8.16, so I installed 2.8.22 and problem persisted. Happens with ALL site displayed in IE 11. Does NOT happen when capturing Firefox window or any other program.
Thanks for the report. We don't have many Windows developers so let's hope someone can have a look at it soon, but I would not hold my breath. By any chance, have you ever tried the development version of GIMP? There is an installer (64-bit only) there: https://www.gimp.org/downloads/devel/ I am not sure there has been major internal changes of the Win32 implementation for screenshot but it may be worth trying to at least know for sure if your problem is fixed in our current development code or if it is still present. Thanks.
I just tried it myself and neither 2.8.22 nor 2.9.8 can screenshot the IE Window correctly. My guess is that it has to do with the gpu-accelerated rendering most web browsers use nowadays. Because of this, I did some tests with other Browsers: IE: Same as in the screenshot from the report. There is an option to switch off hardware acceleration, yet it did not seem to have any effect on the screenshot result. There is always an empty window without the website. Chrome: HW-acceleration enabled results in a completely black image when taking a screenshot. With HW-acceleration disabled, it is possible to take a screenshot and the result looks good. Firefox: HW-acceleration setting has no impact on screenshots - it always works.
The platform-specific code lives in https://git.gnome.org/browse/gimp/tree/plug-ins/screenshot/screenshot-win32.c
I looked into this yesterday and did some research: 1. finding: taking a screenshot of the problematic applications works when using the "whole screen"-setting of the screenshot tool (so this might be a temporary workaround for you, Sara). So to me it looks like some kind of device context problem. The device context for the selected window is retrieved in line 472 of screenshot-win32.c. The device context for the whole screen is retrieved in line 478. So a viable option might be to use the whole screen device context all the time and just change the coordinates of the area that we copy over via BitBlt in line 411. According to https://msdn.microsoft.com/library/windows/desktop/dd183490%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396 , the CreateDC-function will return a Context for all monitors if there are more than one, so this should also work if the window is not on the main monitor. Or even overlaps two monitors. After reading this, I noticed that there seems to be another bug in the screen capture plug-in btw. The "capture whole screen"-setting only captures my main monitor. But since CreateDC is supposed to return a context for all connected monitors, it should actually capture all three monitors if I understand this right?
Hi Simon, You seem to be on-top of things. Patches are most welcome if you can cook up some improvements to the Win32 implementation for the screenshot plug-in. :-)
I will try to come up with something, it may take some time though since I have multiple exams in the next weeks :)
Take your time. If we have a patch fixing issues at the end, it's worth it. :-)
Created attachment 368830 [details] [review] Use display device context instead of window device context when capturing a single window Ok, this went better than expected - I just played around in my break and this patch already seems to work. There is one downside though: Since this uses the display device context instead of the captured window's device context, it is no longer possible to capture stuff that is off screen. Trying to capture a window that is partially off screen will result in a black area in the screenshot where the window was not visible on any screen. I will do some further research in order to find a solution for this problem. Also I will try to solve the second problem regarding multi-monitor full-screen screenshots I mentioned in my last answer when I have time.
Thanks Simon. That's great! Could you do the following when you do a patch: (1) Create a local commit with your name, email and comment. (2) Run: git format-patch origin/master This will generate a file for your commit which you can upload here. This way, we have directly the appropriate authorship and comment for a patch. (In reply to Simon Müller from comment #8) > There is one downside though: Since this uses the display device context > instead of the captured window's device context, it is no longer possible to > capture stuff that is off screen. Trying to capture a window that is > partially off screen will result in a black area in the screenshot where the > window was not visible on any screen. > > I will do some further research in order to find a solution for this problem. Can't you just use the old implementation for Window screenshot and the new code for screen screenshot? > Also I will try to solve the second problem regarding multi-monitor > full-screen screenshots I mentioned in my last answer when I have time. Cool. Feel free to do any worthwhile improvement to this plug-in! GIMP's Win32 code needs some love.
Created attachment 368832 [details] [review] Use display device context instead of window device context when capturing a single window > Could you do the following when you do a patch: > (1) Create a local commit with your name, email and comment. > (2) Run: git format-patch origin/master Done :) > Can't you just use the old implementation for Window screenshot and the new > code for screen screenshot? I don't know if I misunderstood you but the new code is for Window screenshot and not for the screen screenshot (which already worked). The old implementation of the Window screenshot grabbed the window's device context and copied the content of this DC to a local buffer. Some applications that use hardware acceleration don't use this DC though and render to a different DC. So the old implementation could only capture those parts of the window that were actually rendered to the main DC. Everything from the secondary DC was omitted. The new implementation now uses the display DC even for Window screenshots because the display DC contains everything exactly as you see it on the monitor (but only that which is the problem I mentioned above).
You guys are great! Yes, the capture whole screen only capturing one monitor is part of why the failure to capture a single window was particularly irksome. Thanks for all your work.
(In reply to Simon Müller from comment #10) > > Can't you just use the old implementation for Window screenshot and the new > > code for screen screenshot? > I don't know if I misunderstood you but the new code is for Window > screenshot and not for the screen screenshot (which already worked). Oups yeah sorry. I am not sure why but I mixed up screen/window. :-) > The old implementation of the Window screenshot grabbed the window's device > context and copied the content of this DC to a local buffer. > > Some applications that use hardware acceleration don't use this DC though > and render to a different DC. So the old implementation could only capture > those parts of the window that were actually rendered to the main DC. > Everything from the secondary DC was omitted. > The new implementation now uses the display DC even for Window screenshots > because the display DC contains everything exactly as you see it on the > monitor (but only that which is the problem I mentioned above). About the issue you mentioned for out-of-screen window parts, instead of using the display DC, can't you just grab the Window's device context as well as the hardware-accelerated DC and blend them together? Can there be any arbitrary number of such DC ending-up in the display or are there a fixed known number of them (or simply an API to get the full list of DCs)? Note that I am just throwing random ideas since I have no ideas how this all works on Windows. Anyway your code looks good to me, so I decided to push it. There were just a few trailing whitespaces but I just cleaned these up myself and amended the commit (we are a bit uptight here on these details and code "cleanliness", just so you know for future patches! ;p) Even with the regression for out-of-screen windows, this can likely be considered an improvement already. I still leave this bug report opened to hopefully get further patches (from you hopefully! ;p) to handle the regression part.
Review of attachment 368832 [details] [review]: Pushed as commit: commit 14236761cdf528f4a7493a1c9f781c7909687923 (HEAD -> master, origin/master, origin/HEAD) Author: Simon Mueller <s.mueller.hn@web.de> Date: Fri Feb 23 16:39:13 2018 +0100 Bug 793722 - Capture screenshot of single window fails if... ... thewindow is IE 11. The screenshot plugin for windows had a problem when capturing applications that use hardware rendering acceleration (e.g. Chromium-based Apps, IE11). Those applications seem to render their content to a device context (DC) that is different from the one that can be retrieved via GetDCEx(hWnd). So a screenshot that simply copies the main window DC will be incomplete (see bug attachment) or just plain black. This patch removes the code that uses GetDCEx for single window screenshots and always uses the display device context instead. This makes sure that all window contents are actually visible in the screenshot. With this change, we now have to set the source coordinates in the call to BitBlt() to the window's coordinates to exclude everything that isn't the window from the screenshot when doing a single window screenshot. Review comment by Jehan: as Simon notes in bug 793722, there is a regression though, which is that this new code cannot capture any part of a window which is not in any screen. This is still an improvement because at least for what is on screen, we always get exactly the same as what is displayed. This is especially true since hardware-accelerated applications are more and more common. So let's push this first commit and hope for further improvements. plug-ins/screenshot/screenshot-win32.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-)
By the way, are other Win32 screenshot apps able to get a complete window screenshot for IE11 (or hardware-accelerated Chrome, or other similar software) even when out-of-screen? We may not look at their code if they are not Free Software, but at least we'd know a solution exists somewhere. :-)
Regarding the regression: I just checked whether this problem also exists in OBS Studio's 'window capture' media source since they also use BitBlt (https://github.com/jp9000/obs-studio/blob/master/plugins/win-capture/dc-capture.c line 156) for that and indeed it does (missing website content with IE11, black screen with Chrome). I will play around a little bit more but I am slowly coming to the conclusion that it is somewhat impossible to capture a hardware accelerated application with GDI. I will look into the various other options like direct3d as mentioned in https://blogs.msdn.microsoft.com/dsui_team/2013/03/25/ways-to-capture-the-screen/ when I have a bit more time at hand.
Sara > could you tell us if SnagIt is able to take a full window screenshot even when the window is partially out-of-screen (out of any-screen even, if you have several screens)? Same question if you have any other screenshot app. Thanks!
Created attachment 368877 [details] [review] Adjust source rect for full screen capture so that multiple monitors get captured correctly Had another break from learning and managed to fix the multi screen issue I mentioned above (at least I hope so :D).
Oh I somehow overlooked your suggestion that we could blend together multiple buffers yesterday, Jehan. So sorry I did not answer until now. I had the same idea and did some research on how to get a handle to the hardware accelerated context but I did not find anything useful yet. I have no idea if it is even possible but will continue my research a bit. If I can't find any clue on how to do it the GDI way, I will look into the direct3d stuff I mentioned above.
Review of attachment 368877 [details] [review]: Thanks. I will push. This is actually fixing bug 789728, right? :-)
Review of attachment 368877 [details] [review]: Ok I have pushed your commit and amended the description for bug 789728. Also if you check the patch contents, you'll see I amended your code for minor syntax fix: we add a space between function name and opening parenthese; and I aligned a bit the code (which is not mandatory, but this is a pretty common convention in our code base). Thanks for this! commit 653798146e9ab5b0f8455edd4573d1c70d45bb0f (HEAD -> master, origin/master, origin/HEAD) Author: Simon Mueller <s.mueller.hn@web.de> Date: Sat Feb 24 17:23:05 2018 +0100 Bug 789728 - Screenshot whole screen (Windows 10) only grabs screen... ... from first monitor While researching the cause for the missing window contents (bug 793722), I noticed that the full screen capture mode was also not working as expected. No matter how many monitors were connected, it only ever captured the contents of the main monitor. This patch adjusts the source rectangle for the BitBlt copy operation so that multiple monitors are captured correctly.
(In reply to Jehan from comment #14) > By the way, are other Win32 screenshot apps able to get a complete window > screenshot for IE11 (or hardware-accelerated Chrome, or other similar > software) even when out-of-screen? > We may not look at their code if they are not Free Software, but at least > we'd know a solution exists somewhere. :-) Hello, about your question, The answer is yes. I am the developer of this: https://www.youtube.com/watch?v=QaMUjMtIz_g Building it required me to deal with exactly the same problems. I had to think of a way how to do screenshots to windows of these problematic apps. I found a way to solve it and make it done. and this also includes a situation where the window is off the screen or covered by another window. I want to contribute. But there are some problems: I can not figure out how to build GIMP on Windows 10. The process seems unclear. This prevents me from offering help. Unfortunately, I do not have much time to contribute. So you need to make this process as simple as possible
Hello Gil Eli, We have this wiki page about building GIMP for Windows: https://wiki.gimp.org/wiki/Hacking:Building/Windows Apart from this, I cannot really help since I don't build on Windows (well I sometimes cross-build *for* Windows, but from a Linux machine). I hope you'll make sense of it. If you really don't manage to build, you may also explain how you did it, or even send us a link to your code (is it Free Software?). This way, someone with some experience in Windows development may be able to reproduce a similar solution. :-)
I would be very happy to contribute the easy and the fun way (Which is if I manage to build the project). I first want to know, if I explain how to solve the problem but I do not the one that write the solution code, will my name still appear in the contributors list? My software is not open source. But I can write a Passado code that shows the solution.
Your name will appear as the code author of the respective git commits, if you want it to. Pseudonyms can be used if you do not want or can't reveal your legal name. Please note that any code contributed to GIMP will be redistributed under the terms of the GNU General Public License v3 or any later version.
Because the solution must built under visual studio (it use function that implemented in Visual C++ Redistributable), and gimp is not built under visual studio, The solution requires that the whole process of capturing window will be performed in a dedicated exe file(that is - a separate process) that build with visual studio. that separate process will require that Visual C++ Redistributable 2017 will be installed on the user PC (in order that it will do the job of capturing window). It can work also with some previous versions of Visual C++ Redistributable but I did not tested it. I can develop the separate process that capturing window and provide it's source code. but because I can not build gimp on my PC, I will not be the one to do the integration with gimp. The plan is that I will develop the exe file that make the job done, you will get the source code. then someone will edit the code, to make the integration (so that gimp will able to send commands the that process and receive the output (using some communication mechanism between processes). Another request: The name of this exe file will contain "gileli". for example, "gileli-capture-helper.exe" or something like this. Do you agree with the plan?
Are you sure that Visual Studio is required, or is this mostly due to you using Visual Studio exclusively? Also, the current screenshot code already lives in a plug-in which is run as a separate process, so there shouldn't be much difference there. Having your name as part of the file name is understandable, but might achieve less than what you think it will - many people never ever see these. You can get much better exposure via commit comments (which will show up on e.g. openhub.net) and announcements about your contribution. I would very much prefer to keep file names as they are now.
Also, your name would appear in a release announcement, and permanently in GIMP's AUTHORS file, and scroll by in the "About" dialog.
"Are you sure that Visual Studio is required, or is this mostly due to you using Visual Studio exclusively?" I'm 95% sure of that. My suggestion is that first we start building what I know can work (based on proven experience) - That is building it with Visual Studio. Then you are welcome to try making it work without it. I understand that the process win-snap.exe is responsible for the GUI part + the screenshot part. Did I understand right? Since it is necessary to do the capture process under Visual Studio, The most logical thing to do is to do it under separate process. that is a "helper process" for win-snap.exe. so that we don't need to move all of the project to Visual Studio. but we only need to move the part of capturing *single* window to Visual Studio. So that when the user will capture single window, win-snap.exe will run "win-capture.exe" (something like this). then, "win-capture.exe" will send the the array of the pixels(the image data) using https://msdn.microsoft.com/en-us/library/windows/desktop/ms649011(v=vs.85).aspx (Unless you have a better idea of how to send the data. I'm just saying that this is a method I've already tried and I know from experience that process A can send the picture to process B through WM_COPYDATA message) My recommendation is that win-snap.exe will do the capture but when it comes to capturing specific window, it will done in a separate process that I propose. The added benefit is that I do not have to look at things that I should not look at (such as things related to the GUI). This will make it easier to work. I need a starting point. I suggest it works that way: I'm going to build a process which gets parameters. Suppose the name of the process is "win-capture.exe", "win-snap.exe" will have to call it with parameters as shown in the example: win-capture.exe 0xAAAAA 0xBBBBBB parameter 0xAAAAA is the handle of window in win-snap.exe that will receive the WM_COPYDATA message when the capture is done. 0xBBBBBB is the handle to the window that win-capture.exe need to capture. That's my suggestion. Another possibility is that "win-capture.exe" will save temp file that is the image of the window, and win-snap.exe will take it from there.. Anyway, I said from the start - since I have not yet managed to build gimp then the part of integration is less suited to me. I'm just describing how I think this should work .. I'm just talking in general, suggesting ideas about the issue of integration. (I need it for the background... the mechanism of communication someone else can implement) And I want to know where to send the code. I'm a bit new about github so I do not really know how to send the code
No, win-snap is dead, you are probably looking at old stuff from a 2.8 release. We are releasing 2.10 right now, and generally you should be using git master for development, but the 2.10.0 tarball will do too. These days the plugin is called "screenshot" and shares common code across platforms, with a platform-specific backend part, it's all in gimp/plug-ins/screenshot/
That explains a lot.... I just did not understand why there was no correlation between the code I saw and the software I downloaded. Sorry for the mistake. I need to do some homework about this. I will use build: https://download.gimp.org/pub/gimp/v2.10/windows/ For better understand how it works (beyond the code I see)
"Also, your name would appear in a release announcement, and permanently in GIMP's AUTHORS file, and scroll by in the "About" dialog." I understand correctly? - that if the code you will write is based on my code, then you will do what you specified in the quote. Please confirm that. What I'm doing now is building working example. I think that's the quickest way to go forward.
Hi Michael, Jehan, After some thinking, I came to the conclusion that actively contributing to such a big scale open source project like gimp would be the best course of action for me as an individual. I will take this opportunity to try and complete the full cycle for solving this bug, which inlcudes building natively or Windows, running and reproducing the issue, providing a patch, and most importantly receiving code review. I have help for building for Windows *on* Windows using the MSYS2 guide and cross compiling *for* Windows on Ubuntu, so that won't be an issue. If I stumble across difficulties and can't proceed, I will publish a working example instead (I already have such an example). Best regards, Gil Eliyahu
Quick update - I succeeded to compile gimp on Windows10 using MSYS2 instructions, reproduce the bug and I started working on a solution. Will update in a few days.
Awesome. :-)
Update: I started to integrate my capture sub system with screenshot-win32.c I managed to capture the window in my capture-window.exe process (written in C++ and microsoft visual studio) and then send it to screenshot.exe now when the parameter selectedHwnd in doCapture (HWND selectedHwnd) is not NULL, the function will call to capture-window.exe to get the capture. Then capture-window.exe sending back the pixels to screenshot.exe. The problem is that something is not working right when gimp is rendering the pixels array. look: http://prntscr.com/jbj52k (As you can see, part of the window is out of the screen) I do not understand what can cause the problem. I had to convert an array of pixels that have RGBA colors to RGB only. I don't think that I did something wrong in the code. this is the test code that convert RGBA to RGB unsigned int pos = 0; for (unsigned int i = 0; i < size2; i += 4) { capBytes[pos++] = capturedPixels[i]; capBytes[pos++] = capturedPixels[i+1]; capBytes[pos++] = capturedPixels[i+2]; } Without this code, the result is much worse. Maybe there's something I'm supposed to know that's causing the problem?
Looking at the screenshot, it looks like you are doing the right thing RGBA->RGB wise, but you may be using the wrong width of original image (row stride), which explains why the pixels are progressively being shifted one line after another. Maybe there is some padding in whatever is returned by Windows? I can't say much without knowing more of the code (and possibly testing).
Then you, the problem has been solved: http://prntscr.com/jbomvv It will take some time until I will release the path.
Here's a video that shows the bug in gimp v2.10 that you just released and from time 0:40 you can see that the bug has been fixed. https://www.youtube.com/watch?v=PllunZU726
I accidentally gave a wrong link. This is the right link: https://www.youtube.com/watch?v=PllunZU726I
Hello, I'm new at github and I made a mistake. It leaked as pull request named "bug 793722 windows10 screenshot" I did not mean it to be released at this point. In any case, the patch works. But it's not perfect yet. I still have work to do. The patch works. I would like to note that it uses capture tool that I wrote "gileli121/capture-window". The code of the tool is not yet clean, and there are some bugs that need fixing. There is more work to do. Anyway the code of the tool does not have to be included in gimp in my opinion. in such case the only code that will be included in gimp will be any code related to the integration with the tool.
Hello again! Here is the patch: https://gist.github.com/gileli121/ea64b5d6391d89b7bf4a9ce7c66e168a You have 2 files: Magnification-win32.h screenshot-win32.c I managed to get rid of the need for an external exe file. Everything works natively in screenshot-win32.c . Note that in order that the patch will work, Microsoft visual c++ redistributable 2013 or later version of it must be installed on the PC. In addition, it has a small drawback - it use function with the status "deprecated". it is the status since 2013. So it can be assumed that if Microsoft were to delete this function, they would probably have done it long ago. Anyway, I designed the code in such a way that if it failed to do the capture for any reason that can be, it will capture with the normal method. There had to be a lot of change. I hope this is a great contribution for you! I am happy to contribute code to the community and especially to such a project. If you accept my contribution, do not forget to put my name as you said :) have a nice day! Gil Eliyahu
I just fixed something in the code. If you downloaded, download again. I am waiting for a review.
Could you attach the patches to this bug report, please?
Created attachment 371766 [details] Patch that fix the "out off screen" bug, without any need of moving the window This patch that fix the "out off screen" bug, without any need of moving the window. The entire window will be captured even if part of the window is off screen. There is no need to move the window front & to be not "off screen", the patch use complicity alternative mechanism for doing the window capture - It use the magnification api: https://msdn.microsoft.com/en-us/library/windows/desktop/ms692162(v=vs.85).aspx Note that in order that the patch will work, Microsoft visual c++ redistributable 2013 or later version of it must be installed on the PC. In addition, it has a small drawback - it use function with the status "deprecated". it is the status since 2013. So it can be assumed that if Microsoft were intended to delete this function, they would probably have done it long ago. Anyway, I designed the code in such a way that if it failed to do the capture for any reason that can be, it will capture with the normal method.
Note that this is the first time I have contributed code to an open source project. If you have feedback about the code, I'd love to hear. Gil Eliyahu
Comment on attachment 371766 [details] Patch that fix the "out off screen" bug, without any need of moving the window The path is a zip file that contains the files Magnification-win32.h screenshot-win32.c You need to save it as zip file. And I need to correct one of my sentences (sorry for the English): The patch use *completely* alternative mechanism for doing the window capture.
Comment on attachment 371766 [details] Patch that fix the "out off screen" bug, without any need of moving the window I just want to correct myself: This patch does not requires any version of microsoft visual c++ redistributable. This is no longer true because I found a way to avoid having to build a separate executables file microsoft visual studio. The only requirement of the patch is that the dll file "Magnification.dll" Will be exists on C:\Windows\System32 folder. this is no problem because this dll file comes with every clean installation of Windows. But again, it uses the function "MagSetImageScalingCallback" that stored inside this dll. The status of the function is "deprecated" since 2013. In the event that the function will be removed in newer versions of this dll, the function in my Magnification-win32.h file that is "LoadMagnificationLibrary()" will return FALSE because the variable/function at line 143 - "MagSetImageScalingCallback" will be NULL. In such a case, in line 607 under the function doCaptureMagnificationAPI(), it will return FALSE. As a result, the expected outcome is that the function doCapture() will call to doCaptureBitBlt(selectedHwnd, rect); at line 489 Bottom line: A crash error is something that should not happan in case we call to deleted function because the code will not call to deleted function in the worse case scenario. I do not think Microsoft will delete this function. It can be assumed that if they were to delete the function, they could have done so long ago. In any case, this solution takes into account such a situation. In which case it is supposed to the capture with the old method. There is a way to ensure that the function is never deleted - that is if gimp will come with this dll file, whre the plugin is located. But the question is whether it is allowed to distribute Microsoft dll files. There is a legal aspect here. If there is no such problem, then the perfect solution is for the plugin is if the Magnification.dll file will come with the plugin. so in case that MS will delete the function in new version of the dll, it will not effect the plugin because it use old version. I just note it as a possibility .. give directions of thought how to make it better
Created attachment 371896 [details] [review] Patch by GilEli cleaned up. Thanks for the patch. I have many comments of coding style and "how to contribute" tricks, which explains why your patch is not taken into account immediately: (1) Do not upload files! Upload patches! And in particular you must upload "git-formatted" patches, especially since you want to keep your authorship so badly (which is a given for us, by the way, we always keep everyone's authorship). Basically, in git, you should have run: > git add plug-ins/screenshot/Magnification-win32.h > git add plug-ins/screenshot/screenshot-win32.c > git commit Then add a proper commit message explaining what your patch does. This way, this commit will contain your name and email (assuming you set up git accordingly)! This is the base of keeping authorship! Then once this is done, run: > git format-patch -1 This will create a patch file which you can upload here and make it a lot easier for a first quick online review. Well I've done it for you here, and I used "GilEli" as name and the same email address as on bugzilla. Is that right? Or did you want to give another authorship (like your full name)? (2) Set your code editor to use Unix-style line endings (and not DOS' ones). Right now a diff looks like you changed every lines of the file which makes it very hard to review (then later to track). I could fix these with `set ff=unix` in vim. (3) Clean out all end-of-line spaces. You should always set your editor to show them to you. (4) Do not use tabs for indentation but spaces. Exact indentation rules can be found in the "Hackordnung" section of HACKING file at root of the repository. (5) Use our indentation rules, in particular I can see you align your braces with the previous code, which is not our coding style. We indent them one level (same as the GNU coding style for instance). There are even lines which you didn't touch but reindented wrong. Don't do this please (you can "fix" indentation, but don't make it wrong ;p). (6) Add spaces before parentheses, and not after. This is also explained in HACKING file. (7) Normally you should use lowercase names with underscore, but this file was already using camel-casing, so I guess it is ok to keep this style at least temporarily. But then keep consistent by having the first letter as lowercase as other functions (which you did correctly for all but one of your new static functions). (8) When defining functions, don't forget the "static" keyword (especially since you did forward declaration with static), and put the return type on its own line before the function name. (9) Align the function parameters in function defintion too, please. (10) Don't edit random parts of the code which you are not actually touching. I notice that you added or removed spaces or newlines here and there (without actually doing any real functional changes), which makes reviewing harder because the diff is bigger. Don't do this. (11) Don't add C++-style comments (//), we only accept C-style ones (/**/). And probably other things. Basically, for things not explicitly written in `HACKING`, look at existing code and copy its style. It is very important for a source to stay consistent. Otherwise if every contributor would mix in one's own coding style, the project would become an unmaintanable mess. :-) I have tried just now to review your code, but it was simply impossible from its current state (every lines was shown by git, for one). So instead I did some cleaning. Actually I should have asked you to do it, not me. But well… too late. So here is your cleaned-up patch. I'm sure I missed stuff, and in particular in the new `Magnification-win32.h`, I only did the basics (space and tabs), but alignment should be fixed at some point too. Please if you make any functional fix, work from this cleaned-up version, not from yours (to avoid having to redo all the cleaning). Other than this, I have not done any proper functional review yet. I only checked that it indeed builds well (I just crossbuilt locally with this code) but did not even test in a VM. That's a good thing, since a build dependency on Visual Studio would have definitely been a blocker. If anyone wants to do the review, please be my guest. Simon Müller, maybe? You seem like you know your way around Windows.
Oh, sorry .. I did not take too much into account the coding rules. I just I just focused too much on providing a working solution. I had to pay more attention to coding style.. it was the first time I have been participating in an open source project. from now on I will pay attention to the writing rules (coding standard) and especially what you mentioned. Please change the name from "Gil Eli" to "Gil Eliyahu". Thank you!
Hi! I have tested in a VM and that works quite well. Good work! I still found a case where it fails: if the target window is maximized, then if some other window is above it, it will capture the part of the window above. I see you do a check in your code about the maximized state (I have not looked the code in detail yet, so not sure if that is the actual cause), why is that? Is there a problem and this API you use would not work when the target window is maximized? Or is it just a bug? In which case, if you can fix it, it would be awesome. :-) In any case, great work. I will still review the code, just for sanity of mind. But that is a good job anyway. If you send an updated version, please work from the patch of your cleaned-up code, so that I don't have to redo all the cleaning work, please. :-)
"I still found a case where it fails: if the target window is maximized, then if some other window is above it, it will capture the part of the window above. I see you do a check in your code about the maximized state (I have not looked the code in detail yet, so not sure if that is the actual cause), why is that?" Oops, this is not supposed to happen. As for your question, There is a function called "MagSetWindowFilterList": https://msdn.microsoft.com/en-us/library/windows/desktop/ms692396(v=vs.85).aspx I use the mode MW_FILTERMODE_EXCLUDE because according to the documentation, MW_FILTERMODE_INCLUDE is not supported in several Windows OSs It forces me to add windows that I do not want to include in the screen capture. and this is instead of specifying one window I want to be included in the capture. I can add up to 24 windows. more than 24 windows, the api failed according to my tests. so I do not want to add all the visible windows. I only add windows that are above the target window. That way I avoid as much as possible from reaching more than 24 windows in the list of windows to exclude. So the code need to check if the window is 1) Above the target window 2) And also cover the target window (According to the position and size of the window). If both conditions are met, I add the window to the list. The reason why I check if the target window is maximized is because in case it is maximized, I don't need to check the second condition. I just assume that if the target window is maximized, then any window above the target window also "cover" the window. Now that I think about it, The assumption is incorrect - the maximized window can be on different screen. In the case of the bug in the patch, that I was able to reproduce on my computer, the code did not add the windows that above the target window. There is a logical error in the code that I just found. I feel very bad that I did not test this scenario.. In any case, again - my assumption was incorrect . I'm not supposed to assume that every window above the target window is also cover the target window, when the target window is maximized. instead, my code should check condition 2 anyway: That means that in line 661 I should only use isWindowIsAboveCaptureRegion(*) and if it return TRUE then add the window to the list. so I need to remove the part of isMaximaized in that loop. it will not fix the bug. I checked the source of the bug and found it: There is a logical failure in the loop itself - for some reason, the function GetNextWindow will not return the windows that are above the target window, in case that the target window is maximized. it is not problem in the api. I can fix it easily. Do not worry :) Anyway, I still need to check if the target window is maximized for another reason: according to my tests, there is a bug that if the window is maximized and I capture the window then I get this problem: http://prntscr.com/jh5qrx Again, do not worry :) I know this bug with this api, and I solved it when I developed my software. It happens if the target window is maximized.. so the code need to check it and if so then it need to do some fix that I know how to Implement. I knew that this patch is not perfect yet. I just wanted to see your response before I continue working on it.. I would like to point out that this is not the only way to solve the problem - I can also write function like "doCaptureMagnificationAPI" but the new function will do the capture in different method, using undocumented windows api. This is an option .. Anyway, first I have to finish working on "doCaptureMagnificationAPI" and make it perfect. "If you send an updated version, please work from the patch of your cleaned-up code, so that I don't have to redo all the cleaning work, please. :-)" Yes, I will use the cleaned patch you sent.
> I just wanted to see your response before I continue working on it. Well it definitely looks like you know what you are doing, you are using an official API of Microsoft, it builds with our current build system, etc. Basically that's good work. So yeah it will definitely go in, no problem there. :-)
Created attachment 372076 [details] [review] Fix "out of screen" bug when capturing window and bug 796121 This is an updated version of the patch. The following bug has been fixed: * If the target window is maximized then the capture also include windows that above the target window * If the target window is maximized then there is this problem: http://prntscr.com/jh5qrx * The bug https://bugzilla.gnome.org/show_bug.cgi?id=796121 And also: * Added comment in the code describes what the code does. * I tried harder to keep the standards. * This patch is based on the cleaned version by Jehan. I hope it's better now. Waiting for functional review and testing. The patch is fully functional, with no new problems found.
Review of attachment 371896 [details] [review]: Obsoleted by the new patch by Gil.
Review of attachment 372076 [details] [review]: Pushed! I only did a bit more cleanup (some trailing spaces in particular), but otherwise that's very good! commit ae93b6db187cfc949d06f08a62effdf811199719 Author: Gil Eliyahu <gileli121@gmail.com> Date: Tue May 15 18:04:19 2018 +0300 Plug-ins: fix screenshot bugs in windows. This fixes bugs 793722 and 796121. In particular it fixes: - Single-window screenshot when partly off-screen or covered by another window. - Screenshots when display scaling is not 100%.
And other than the minor cleanup I did in your commit, I did some more work in these additional commits: commit a0b28589acbe5fc901264a3bbe44e73a86aa18dd Author: Jehan <jehan@girinstud.io> Date: Tue May 15 22:18:11 2018 +0200 plug-ins: properly load user32.dll for SetProcessDPIAware(). Using similar code as other parts of Win32 code. I hope I am not doing anything wrong. At least now it builds! commit 382d6c8ad5b66719cc906501e0d3f92c7b55feef Author: Jehan <jehan@girinstud.io> Date: Tue May 15 23:32:55 2018 +0200 plug-ins: put the initial foreground window back after a screenshot. This was lost in commit 966843564d. It's not a big deal since this code path would only happen when the capture using magnification API fails, yet we may as well make it perfect. Also taking the opportunity to change the return type to gboolean for the various capture functions (though it is technically the same, semantically we were returning success boolean). And removing a comment which had been duplicated and left at a the wrong place. commit e357e7118ce106092fc575afe89a4c6b246fa831 Author: Jehan <jehan@girinstud.io> Date: Tue May 15 23:50:10 2018 +0200 plug-ins: fixing various compilation warnings. Mostly warnings about wrong types for some function parameters. There is still a single warning remaining about ignoring the #pragma macro, but I am not sure what to do about this warning. Apparently it is something specifically for use with Visual Studio. We don't need this, but since the contributor uses it, let's keep it. commit 9fb1c2868709d1012068756f0682d081537d132c (HEAD -> master, origin/master, origin/HEAD) Author: Jehan <jehan@girinstud.io> Date: Wed May 16 00:24:13 2018 +0200 plug-ins: properly free rectScreens after allocating it. Also avoid global variables when possible. We can just use the data variable of EnumDisplayMonitors() which will be passed on to the callback. This is not perfect yet since rectScreensCount is still global, but let's go for it for now.
By the way, Gil, your patch seems to work very well according to my tests. There is only one detail: when I was taking window screenshots, I noticed a few more pixels outside the window on the border (was easy to spot since the background was bright blue, so I could see this blue line on sides). Do you see this too? If so, would you be able to submit an additional patch? :-) I pushed this one already anyway because that's anyway a lot better than current situation, and also because we want to release a 2.10.2 soon. So I prefer not to wait. But if you could submit an additional patch (when you do, just open a new bug report), we would be welcome. Also as promised, your name is now forever in the git log for your commit (and if you provide more patches, it will be in more commits! ;-). It will also be on the website in the authors list when we will update it. By the way, other than this small fix I asked for your code, we have a lot more Windows bugs and are really lacking of Windows developers. You are really welcome to look into our various opened bugs and upload patches for whatever you want. :-)
(In reply to Jehan from comment #57) > By the way, Gil, your patch seems to work very well according to my tests. > There is only one detail: when I was taking window screenshots, I noticed a > few more pixels outside the window on the border (was easy to spot since the > background was bright blue, so I could see this blue line on sides). > > Do you see this too? If so, would you be able to submit an additional patch? > :-) > I pushed this one already anyway because that's anyway a lot better than > current situation, and also because we want to release a 2.10.2 soon. So I > prefer not to wait. But if you could submit an additional patch (when you > do, just open a new bug report), we would be welcome. About this small issue, I created the new bug 796151. Current report lived too long (already 57 comments!). Let's let it rest in peace. :-)