mbari-org / Sharktopoda

Mac Video Player for creating, editing, and viewing localizations.
Apache License 2.0
2 stars 1 forks source link

Resizing video window causes bounding boxes to slowly animate to correct position #17

Open hohonuuli opened 1 year ago

hohonuuli commented 1 year ago

Video example of this behavior at https://youtu.be/hgCCTw3lIsw

dingosky commented 1 year ago

Possible fix: commit 9f47eeb5b876bd9def2e64d154cd5a8d465d3761

See comment for issue #18

dingosky commented 1 year ago

Proposed fix: commit d2860d620b5427fb824bd33629387a90696ef7b1

For efficiency, the background task resized all localizations, including the current paused ones, rather than make an extra check to see if the localization was paused (and hence previously resized). When the background task hit the on-screen paused localization, well, you saw the result. The code was restructured to prevent re-resizing of paused localizations.

hohonuuli commented 1 year ago

That did the trick.

One minor nit now. When paused and the frame is resized, any currently displayed bounding boxes disappear. Advancing a frame forward and back causes them to reappear.

dingosky commented 1 year ago

Ha! I just hit that too.

dingosky commented 1 year ago

Proposed fix: commit 862d42f846fafe6679cdd4f102f25161b364b6f2

hohonuuli commented 1 year ago

Sad to say that's there still some resizing weirdness going on.

Here's a video demonstrating the boxes not getting updated with the correct positions after resize. (at time 22). The boxes jump from bad to a good position when the frame is advanced: https://youtu.be/SBVmDPAn4VA?t=22

In this video, all the boxes are within the video bounds. However after resizing, they are now displayed a the wrong location and appear out of the resized video bounds. https://youtu.be/ZDsnx94go0M

Are we having fun yet? ;-)

dingosky commented 1 year ago

Ok, here's the processing scenario.

As a window is resized, Sharktopoda processes a stream of resize notification events (this is macOS behavior, not specific to Sharktopoda). Sharktopoda immediately resizes paused localizations (for the user's benefit), and schedules the resize of all other localizations with a short delay. If a subsequent resize notification is received inside that delay, the process reboots, with the current paused localizations immediately resized and the rest scheduled again after another short delay.

Doing this "on the fly" during video playback would be computationally expensive. I suggest that if the window is resized during playback, the video should auto-pause and proceed with the above processing. To be clear, resizing a live video stream isn't the issue, it's having to "resize and relocate" all localizations that will cause the pain.

dingosky commented 1 year ago

And in the future we could do restart playback if appropriate in a manner similar to the way scrubbing works now.

hohonuuli commented 1 year ago

Doing this "on the fly" during video playback would be computationally expensive.

Let's defer this one for now. It's not a show stopper and our users will just have to be aware of it.

dingosky commented 1 year ago

I still suggest we at least auto-pause at this juncture, which will prevent the out-of-bound boxes you were seeing.

hohonuuli commented 1 year ago

I'm fine with that. I'm just trying to be considerate of your time.

dingosky commented 1 year ago

Proposed fix: commit 3c0b04a2ddf20fe61002bf1d3c0751591e6eb3b5

I had actually already implemented this, but it had a minor error in logic (needed a Bool to track the "initial" play state of the first resizing event).

hohonuuli commented 1 year ago

@dingosky Resizing is better and I think it will be fine now. But I'm leaving this issue open as this will be one of the first things the video lab notes. I ran an acid test with 20k localizations and posted a video of it at https://youtu.be/XhAlsF8kiY4 to show the current behavior.

hohonuuli commented 1 year ago

BTW the video play back with all those localizations is <chef's kiss>. This video player is going to be a game changer.

dingosky commented 1 year ago

Some this and that regarding this issue.

First, and foremost, the video window resizing of most apps doesn't trigger processing of a large amount of non-visual data, as we are doing. That processing (the resizing of localizations) is done proactively, at this point, to "prepare" off-screen localizations for viewing as the video plays. With the current implementation, the off-screen localizations are resized randomly (by virtue of iteration over a hash set) and hence may display before being resized.

We could investigate adding a dirty flag to localizations so that when an off-screen location is called for display, it could be resized at that time if it had not already been resized.

We could also investigate resizing the off-screen localizations most likely to be displayed "next" rather than randomly.

We could also, of course, put a modal spinner during off-screen resizing, but I suspect you won't like this idea any more than I do.