Skip to content

Convert Image and Video annotators to TypeScript#483

Merged
subdavis merged 10 commits into
masterfrom
ts-annotator
Dec 10, 2020
Merged

Convert Image and Video annotators to TypeScript#483
subdavis merged 10 commits into
masterfrom
ts-annotator

Conversation

@subdavis
Copy link
Copy Markdown
Contributor

@subdavis subdavis commented Dec 8, 2020

The last major remaining typescript conversion.

Notes to my reviewer

As usual, typescript/composition-api conversion is an exercise in topological sort. This part was particularly tricky. There were some function dependency cycles, and a lot of back-and-fourth between component and mixin.

  • Please pay attention to useMediaController.ts: function initialize() for how I had to solve a dependency issue
  • I'm pretty happy with the new ImageDataItemInternal structure in ImageAnnotator. As mentioned before, we need to hold a reference to both the image and the onload promise. Conversion to promises did indeed make the code easier for me to understand. There's now only a single place in the code where img.onload = is used.
  • I might have broken resume playback behavior where an image is loading, which causes a pause, then it loads, and playback continues.

fixes #474
fixes #480

@subdavis subdavis changed the title Convert annotators to typescript Convert Image and Video annotators to TypeScript Dec 8, 2020
@BryonLewis
Copy link
Copy Markdown
Collaborator

I might have broken resume playback behavior

I know this is still a draft but I was curious. By looking at it I guess if you had a decent framerate image sequence with large images and a slow network you would get a bit of loading->frame->loading->frame stuttering. This is easier to update now that the onload is in one location, you're triggering loadingVideo when a frame is missing and there isn't a difference between cache loading a frame and immediately loading a frame for displaying.

Before there was a difference between a frame seek load and a cache loading of a frame. In the cache loading onload I would check to see if the loadingVideo === true (loading Spinner) and the playing === true then I would check X seconds (this.playCache) into the future based on the frameRate using checkCached and see if all the frames are loaded in that range. If they were loaded it would remove loadingVideo and continue to play. It was important to make sure playCache was less than the cacheSeconds because it would continue to cache ahead and have a bit of buffer when it started playing.

This logic could be moved to the universal onload now and it's gated by the fact it only checks when playing && loadingVideo are both true. The only thing to watch for is that you can only set loadingVideo to false inside of seek when playing is also false, otherwise it is done inside of the onload when enough images have been cached.

I also concede that there is probably a better way to do this that you may discover or already have in mind.

@subdavis subdavis marked this pull request as ready for review December 9, 2020 15:40
@subdavis subdavis marked this pull request as draft December 9, 2020 15:43
@subdavis subdavis marked this pull request as ready for review December 9, 2020 16:40
@subdavis
Copy link
Copy Markdown
Contributor Author

subdavis commented Dec 9, 2020

@BryonLewis I've addressed your comments with a new checkCache function.

I've fixed several annoying bugs with the old version and enabled the loading spinner for all loading conditions. Please let me know what you think.

@subdavis subdavis requested a review from BryonLewis December 9, 2020 16:43
Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not completely done yet, but wanted to just put up a few initial comments. This is much cleaner and the behavior is a lot easier to understand than the previous mess of onloads of differening types.

Most of my comments aren't anything in this version but things I've now notice with more scrutiny. Feel free to suggest we push them to another PR.

I'll do some more indepth reviewing tomorrow morning.

Comment thread client/src/components/annotators/VideoAnnotator.vue Outdated
Comment thread client/src/components/annotators/ImageAnnotator.vue
Comment thread client/src/components/annotators/ImageAnnotator.vue Outdated
Comment thread client/src/components/annotators/VideoAnnotator.vue Outdated
Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really great. I think this should help greatly with those large image single detection projects. Now hitting right on the keyboard multiple times won't queue up the loading and should make it so the final image loads quicker.

One very minor thing that I can add as an issue or into another PR is the z-index of the spinner. Each geoJS map layer increments the z-index of the base container and right now the loading spinner is below the text layer.
image

@subdavis subdavis merged commit bef8d73 into master Dec 10, 2020
@subdavis subdavis deleted the ts-annotator branch December 10, 2020 19:31
subdavis added a commit that referenced this pull request Dec 10, 2020
* WIP

* Finish up conversion

* Modify docs and comments

* More robust image playback

* CheckCache

* cache improvements

* Address comments

Co-authored-by: Bryon Lews <Bryon.Lewis@kitware.com>
Co-authored-by: BryonLewis <61746913+BryonLewis@users.noreply.github.com>
subdavis added a commit that referenced this pull request Dec 10, 2020
* WIP

* Finish up conversion

* Modify docs and comments

* More robust image playback

* CheckCache

* cache improvements

* Address comments

Co-authored-by: Bryon Lews <Bryon.Lewis@kitware.com>
Co-authored-by: BryonLewis <61746913+BryonLewis@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Housekeeping] Convert annotator code to typescript [BUG] Better loading behavior for viewer startup and image load.

2 participants