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

Add screenshot button to video annotator #1004

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

lehecht
Copy link
Contributor

@lehecht lehecht commented Dec 6, 2024

Closes #956.

@lehecht lehecht changed the title Add screenshot button for videos Add screenshot button to video annotator Dec 9, 2024
@lehecht
Copy link
Contributor Author

lehecht commented Dec 9, 2024

@mzur
How can I check if my button is disabled for non-CORS enabled videos? Do you know any websites with non-CORS enabled videos that I could use to test this? Or did you use a different approach for testing?

@mzur
Copy link
Member

mzur commented Dec 9, 2024

On your local instance you could use the BIIGLE landing page video.

@lehecht lehecht marked this pull request as ready for review December 12, 2024 08:06
@lehecht lehecht requested a review from mzur December 12, 2024 08:07
Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

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

I think I prefer not to use provide/inject here as it hides where the data is coming from. Maybe this is a chance to refactor the screenshot button a bit so it gets all data it needs through props. E.g. if it gets the image/video ID as a prop, it does not even need the image/video changed event any more.

The map.init event must stay, though, as the map object must not be passed as a prop. It should never be made reactive.

Comment on lines +584 to +595
fetch(this.videoFileUri.replace(':id', video.id))
.then((res) => {
res.blob().then(function (blob) {
let urlCreator = window.URL || window.webkitURL;
self.video.src = urlCreator.createObjectURL(blob);
});
})
.catch((e) => {
// Access on non-CORS enabled file causes TypeError
this.hasCrossOriginError = e instanceof TypeError;
self.video.src = self.videoFileUri.replace(':id', video.id);
});
Copy link
Member

Choose a reason for hiding this comment

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

This tries to download the whole video. Usually the video is fetched dynamically through a series of HTTP range requests, which is crucial here.

You need another method to detect CORS. Maybe create a small canvas element and try to read some data from the video into it?

imageInfo['currentId'] = biigle.$require('annotations.imageId');
imageInfo['ids'] = biigle.$require('annotations.imagesIds');
imageInfo['filenames'] = biigle.$require('annotations.imagesFilenames');
imageInfo['type'] = 'image';
Copy link
Member

Choose a reason for hiding this comment

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

Where do you use type?

@lehecht
Copy link
Contributor Author

lehecht commented Dec 13, 2024

@mzur

Maybe this is a chance to refactor the screenshot button a bit so it gets all data it needs through props

I also thought about it, but then I would need to refactor the video and image settingsTab, too. The settingsTabs import the screenshotButton, which would then need the same properties as the screenshotbutton. I am not sure, if they should get props that they will not use except to pass them to the screenshotbutton.

@mzur
Copy link
Member

mzur commented Dec 18, 2024

If it's not too much, I think it is fine to pass the props through the settings tab.

Alternatively you could provide the individual properties (e.g. current ID, current filename) instead of a single "files" object. This is at least more descriptive and you can find the location where these are coming from easier than with the generic name. This would still be better than the solution with the event.

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.

Screenshot button for videos
2 participants