techsola / instantreplay Goto Github PK

Frames 21–32 (out of 100) showed a black rectangle where a dropdown menu should be. Then the black rectangle disappeared about when you'd expect the menu to have closed based on the cursor. Then frames 88–91 showed the dropdown menu and moving cursor focus even though the cursor was nowhere close and the dropdown button was not pressed.

The coordinates for the black rectangle were passed to BitBlt to copy from the window frame source bitmaps to the composition bitmap.

Is this a concurrency issue with GDI, where GdiFlush is needed? Was BitBlt completing the draw to the composition buffer very late? But why would it bother clearing the rectangle to black in the earlier frames if it was just going to copy on top of the pixels? There's no other reason that rectangle would have been black except for BitBlt clearing it when it was supposed to do a source copy.

Maybe the reason for the dropdown being drawn over frames 88–91 was a logic bug in the frame buffer management of closed windows. But it doesn't repro in similar scenarios, and no other dropdown windows in the same GIF even behaved this way. It doesn't seem like the current logic contains enough complexity for there to be a corner case that only struck this once. A second reason a logic bug seems less plausible is that the window would not have been open long enough for the circular buffer to have wrapped and started reusing frames. The only windows reusing frames are ones that opened prior to the start of the GIF.

Actually, I think the dropdown window is not closed when it disappears but is just hidden. Then it might have been opened before the animation started, and therefore the circular buffer might be wrapping and reusing frames.

Tenet: wall time of the SaveGif call

The GIF format permits frames to update only a rectangle rather than the whole image.

Reason: Quantizing is taking the most time, and LZW encoding is the next biggest thing. Most of the time in GIFs of user interactions, most of the pixels are not changing. Having fewer pixels to quantize and encode should give a huge performance improvement while saving the GIF.

The required approach will probably be to have two composition buffers instead of one and swap them each time a frame is written, because we don't have access to cursor pixels until we draw them to a composition buffer.

If no pixels changed, no frame should be written and the delay should be added to the previous frame. This will require both the current frame and the next frame to be fully drawn to their composition buffers before the current frame can start being written.

The scan for the bounding rectangle of the change should be seeded with information about where the cursor and window frames ended up in the composition buffer.

Consider uint/ulong-based reads rather than 3-byte reads for both horizontal and vertical scans, perhaps even bigger vectorized reads and comparisons if applicable. If a horizontal scan that is wider than a pixel finds a change, it's probably not even desirable to resolve it down to a pixel unless the extra complexity of doing so makes the entire saving process faster. Having a few extra rows of pixels on the left and right sides is better than taking longer to save.

These have been seen, returned I think from BitBlt. ERROR_HANDLE_EOF was just seen on 03eee5f, so I know that this general problem has not been fixed.

• ERROR_ACCESS_DENIED
• ERROR_INVALID_HANDLE
• ERROR_HANDLE_EOF
• ERROR_IO_PENDING
• ERROR_FILE_NOT_FOUND

Published 12583e9 to collect more information since there is no known rhyme or reason to the repro so far, and it's much rarer on my machine than on others.

Tenet: privacy

Alternatives: report windows (top-level and child) to blur instead of stopping everything on-screen?

It doesn't seem ideal to erase all buffered frames when you pause, so that means that a GIF will need a frame to be inserted for the duration of the pause. It should be self-explanatory so that it doesn't appear that this is what the app actually looked like at the time or that the recording itself failed.

   at void Techsola.InstantReplay.InstantReplayCamera+Frame.Overwrite(DeviceContextSafeHandle bitmapDC, ref WindowDeviceContextSafeHandle windowDC, WindowMetrics windowMetrics, uint zOrder, ref bool needsGdiFlush)
   at void Techsola.InstantReplay.InstantReplayCamera+WindowState.AddFrame(DeviceContextSafeHandle bitmapDC, WindowMetrics windowMetrics, uint zOrder, ref bool needsGdiFlush)
   at void Techsola.InstantReplay.InstantReplayCamera.AddFrames(object state)

Also tripped #7.

Right now if any exception is thrown by AddFrames, which is running in a timer callback, it goes to AppDomain.UnhandledException with IsTerminating = true and there is no way to keep the app running.

It would be better for the app to have a way to recover with or without losing frames and with or without being able to call SaveGif for the rest of the lifetime of the process.

What is possible? What should the default behavior be, and how much should be configurable? It seems bad to not report exceptions, but it also seems bad to take down the app by default. This means that either exceptions should be moved to UnobservedTaskException, which doesn't feel exactly right, or the user should be forced to provide an Action<Exception> handler.

An interesting second-order exception was thrown while handling the exception in #6 because it came through AppDomain.UnhandledException and was handled by calling InstantReplayCamera.SaveGif, effectively while still inside InstantReplayCamera.AddFrames:

System.Threading.LockRecursionException: 'A read lock may not be acquired with the write lock held in this mode.'

If it's safe to allow the same thread to run SaveGif during any possible exception thrown from AddFrames, then the lock should be made upgradeable. If it can't be safe, SaveGif should act as though there are no frames recorded.

There needs to be a way to atomically decide whether to create the stream in the first place, e.g. FileStream, based on whether there will be frames to write to it.

The left and right buttons should be distinguishable. Full on/off frames should be inserted for each click, even if the clicks are much more rapid than the screen capture rate.

During this time, from the perspective of the app UI, there is no cursor.

Replaces #5.

The additional copies should be no threat to the wall time, and this kind of buffering is sometimes needed anyway if there is a need to know the stream length before beginning to write to the ultimate destination, or just needed e.g. because the MS App Center libraries require attachments to be a byte[].

Then returning byte[]? would indicate that sometimes there is no GIF and would require explicit handling of the scenario where there's nothing to save. Either a byte array containing a valid, 1+ frame GIF is returned, or null is returned.

There was an example of typing and using the mouse simultaneously in a modal window and the window behind it, and the window behind it was showing the wrong series of images.

For one idea, double-check whether there could be a failure to grab images from deactivated windows or owners of modal windows, and make sure we don't confuse lack of images with zero time passing. (There's a circular buffer.)

Attempting to call GetDpiForWindow causes EntryPointNotFoundException to shut off Techsola.InstantReplay as soon as it is turned on.

Review all APIs to make sure that they are supported on all versions of Windows that are still in support by Microsoft (Windows 8.1+ and Windows Server 2012+)

https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-gdiflush#remarks says to expect bool-returning GDI functions to return false when the operation is batched. In that case GdiFlush should be called once after all BitBlts are set up to be sure that frame data is actually copied on time, and we should consider a return of false to be success.

   at Techsola.InstantReplay.InstantReplayCamera.Frame.Overwrite(DeviceContextSafeHandle bitmapDC, DeviceContextSafeHandle windowDC, Int32 windowClientLeft, Int32 windowClientTop, Int32 windowClientWidth, Int32 windowClientHeight, UInt32 windowDpi, UInt32 zOrder)
   at Techsola.InstantReplay.InstantReplayCamera.AddFrames(Object state)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.TimerQueueTimer.CallCallback()
   at System.Threading.TimerQueueTimer.Fire()
   at System.Threading.TimerQueue.FireNextTimers()