Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dance captures thumbnails #25748

Merged
merged 13 commits into from Nov 11, 2018
Merged

Dance captures thumbnails #25748

merged 13 commits into from Nov 11, 2018

Conversation

islemaster
Copy link
Contributor

No description provided.

@islemaster
Copy link
Contributor Author

@islemaster
Copy link
Contributor Author

Current problem: It's easy for the thumbnail to be blank when we save it right away, especially if the song is still loading. We may want a different approach to when we capture the thumbnail. I'm considering capturing every few seconds and uploading the last one after reset.

Also need to see if we can minimize how many thumbnails get uploaded - maybe only upload a thumbnail if we've saved since the last one?

@maddiedierker
Copy link
Contributor

@islemaster please take a look! i've gotten pretty stuck on writing additional unit tests for this code because of stubbing troubles, so i'd like to get this in unless you have any concerns about doing that

@maddiedierker
Copy link
Contributor

also want to document testing troubles -- we use a lot of standalone, exported utility methods in this area of the code, which are particularly difficult to test. this issue outlines the blocker i'm running into with it

const blob = thumbnailPngBlob;
thumbnailPngBlob = null;
// Call completeAsyncSave even if thumbnail save fails.
return this.saveThumbnail(blob).then(completeAsyncSave, completeAsyncSave);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For later: If we can stop depending on the thumbnailURL being saved to the channel row, we'd be able to fire these requests simultaneously! I'm not sure why we do that, since it's always in the same place.

Copy link
Contributor Author

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Awesome! One concern. I hope we're able to avoid uploading thumbnails until save on other project types soon!

@islemaster
Copy link
Contributor Author

Any initial information about the performance impact of this change?

@maddiedierker
Copy link
Contributor

@islemaster compared to other project types where we're always calling the files API to save a thumbnail, this is an improvement since we only call the files API on project save, but i don't have any numbers around it since we previously did not have thumbnail creation implemented for dance party

@maddiedierker
Copy link
Contributor

@islemaster refactored to use a time interval (120,000 ms, or 2 minutes) to throttle thumbnail capture for dance party (rather than using tickCount, which was unreliable). i think there is a broken eyes test that i'm going to work on fixing now, but otherwise this should be good to go! let me know if you have any additional feedback

Copy link
Contributor Author

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Looks great 👍 (I can't "approve" because my name is on this PR, but I approve). Are you satisfied with the thumbnails this is producing? I'm a little concerned that the global two-minute interval will capture about the first frame of the song, more often than not, which is often one of the less interesting views.

@maddiedierker
Copy link
Contributor

that's my same concern, but the thumbnails have been okay so far (pasted a couple below). i was thinking about removing the additional throttle and using the 60,000ms/1min throttle already in place, since all it does is capture a thumbnail now rather than saving the thumbnail. what do you think of that?

screen shot 2018-11-09 at 3 58 44 pm

@islemaster
Copy link
Contributor Author

I'd like a throttle on capturing a thumbnail because I think it might be expensive enough we don't want to do it on every frame/event, but I think it could be relatively frequent since we only upload every once in a while. Unless I misunderstand something, I think it'd be fine to update your in-memory thumbnail every 5 seconds or so as long as we don't upload more than once per minute.

@maddiedierker maddiedierker changed the title [WIP] Dance captures thumbnails Dance captures thumbnails Nov 11, 2018
@maddiedierker maddiedierker merged commit 77b5e96 into staging Nov 11, 2018
@maddiedierker maddiedierker deleted the dance-thumbnails branch November 11, 2018 21:09
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.

None yet

2 participants