Video sprites

[mzur]

Resolves #375

Remaining tasks:

• [x] Implement a console command to generate missing sprites of existing (local) videos
• [x] Try to make generating sprites more efficient (with a direct ffmpeg call such as ffmpeg -i '{$path}' -vf fps={$frameRate} /tmp/frame%03d.png which reads the video only once instead of reading it anew for each frame).
• [x] Maybe simplify the config definition in the blade template.
• [x] Fix the issue with ['size' => 'force']. The regular thumbnails are no longer correct with this setting.
• [x] Move the current hover time (now shown in muted color on the left) to the thumbnail element. If no thumbnail is available, only show the time there. Remove the time from the element on the left.
• [x] Fix the bug where wrong thumbnails are displayed after switching videos
• [x] Use v-show to display the thumbnail on the mouse event.
• [x] Update the settings switch to only control the preview thumbnail and not also the hover time display.
• [x] Preload the previous and next sprite for instant transitions
• [x] Maybe use throttle() for the mouse event (e.g. to limit HTTP requests if sprite images could not be found).

[mzur]

I think the console command is not needed because it can just be done manually in the tinker console. Something like this:

Volume::where('media_type_id', MediaType::videoid())
   ->where('url', 'not like', 'http%')
   ->eachById(fn ($volume) =>
      $volume->videos()->eachById(fn ($video) =>
         Queue::pushOn('low', new ProcessNewVideo($video)
      )
   );

[mzur]

Also I think the ['size' => 'force'] issue may be resolved with the custom ffmpeg call. The call will generate files on disk instead of returning the frame as a buffer. This won't use the same code than the old way of generating thumbnails, so the old way can be reverted to not use ['size' => 'force'].

Maybe we could even do it the other way around? The custom ffmpeg call generates the frame files and then a subsample of these are used as the "old" thumbnails before all of the files are combined into the sprites for the "new" thumbnail feature (if you know what I mean...).

[lehecht]

@mzur

The configured size of 360x270 is the box and the thumbnail is scaled until it is completely contained in the box.

So the image should be stretched if the width or height doesn't fit the box size? Because the image in the CSS contain example also has black bars. Or did you mean that Vips should generate images without bars, whereas these bars are shown in the UI if the ratio doesn't fit?

[lehecht]

@mzur Is it right that for short videos only five ($minThumbnails) thumbnails should be generated? Because the thumbnails preview on the volume overview shows black images for the missing ones at the moment. https://github.com/biigle/core/blob/efab8a51231edcd2dcc79096c563e66c6c9bc803/app/Jobs/ProcessNewVideo.php#L254-L255

[lehecht]

@mzur

Maybe use throttle() for the mouse event (e.g. to limit HTTP requests if sprite images could not be found).

What exactly do you have in mind here? My idea was to check the time difference between the mouse movements on the timeline. If the difference is above a certain threshold and the sprite was found, the thumbnail will be shown. (Remark: I work on a different branch for this feature. So the tasks won't mix up.)

[mzur]

The configured size of 360x270 is the box and the thumbnail is scaled until it is completely contained in the box.

[...] Or did you mean that Vips should generate images without bars, whereas these bars are shown in the UI if the ratio doesn't fit?

Partly, yes. The images should be generated without bars in all cases. With the (old) video thumbnails where the clickable element has a fixed size, there will be black bars in the UI but these are added dynamically with CSS and are not included in the thumbnail file. With the (new) preview thumbnails from the sprites, we can adjust the size of the element that shows the thumbnails above the video timeline. This element doesn't need a fixed size. IIRC it was already implemented like this before your changes.

Is it right that for short videos only five ($minThumbnails) thumbnails should be generated? Because the thumbnails preview on the volume overview shows black images for the missing ones at the moment.

Good catch. Let's just take the videos.thumbnail_count config value as the new minimum.

Maybe use throttle() for the mouse event (e.g. to limit HTTP requests if sprite images could not be found).

What exactly do you have in mind here? My idea was to check the time difference between the mouse movements on the timeline. If the difference is above a certain threshold and the sprite was found, the thumbnail will be shown. (Remark: I work on a different branch for this feature. So the tasks won't mix up.)

This is about what to do if the sprite is not (yet) found. Either something went completely wrong or the video was not completely processed yet. So the current code retries fetching the sprite if the previous request turned out empty. But if you do that on mousemove, you send a huge amount of HTTP requests in a short time. The current code limits this to a fixed number of retries for each sprite but these can still happen very quickly, especially if you move back and forth across the threshold between two sprites. I was suggesting to improve this by limiting the retry requests to maybe one per second, using the throttle() helper function.

[lehecht]

@mzur My code is ready to be reviewed.

[lehecht]

@mzur On the images below, you can see how I added the hover time to the thumbnail canvas. Can you tell me, if you like it or not and what I should change?

[mzur]

I like this more where the time is not overlaid over the video: Please center the time, give it a little padding and show it at the same size than the time of the video timeline on the left.

[lehecht]

@mzur The code can be reviewed now.

[lehecht]

I like this more where the time is not overlaid over the video. Please center the time, give it a little padding and show it at the same size than the time of the video timeline on the left.

Do you like this?

[mzur]

Do you like this?

Please use a solid color for the background (i.e. disable transparency). Also the whole element must move a little upwards but I think my recent commit that changed the positioning should already do this automatically. Finally, is the text color white? Please use the same color than the current time on the left. Otherwise it looks good :+1:

Does it also work if no preview thumbnails is available and how does it look then?

[lehecht]

@mzur I fixed some bugs including the bug that I told you about some weeks ago. Can you check if everything works now?

[lehecht]

@mzur

There was still an issue with the size of the displayed element, which I quickly fixed.

Now it can happen that the hover time doesn't fit on the thumbnail any more, because it's too long. What should I do with such cases?

[mzur]

Now it can happen that the hover time doesn't fit on the thumbnail any more, because it's too long. What should I do with such cases?

You mean if the thumbnail is too narrow? Then you can set a CSS min-width on the element and also center the (narrower) thumbnail in this case.

[mzur]

I fixed some bugs including the bug that I told you about some weeks ago. Can you check if everything works now?

It works alright when switching videos. I'm now a little concerned about larger volumes because the controller has to do so much work and there will be lots of data in the HTML. I'm pretty sure that the thumbnail dimensions can also be inferred from the size of the sprites in the JS. In the thumbnailPreview component you have the spriteIdx, estimatedThumbnails and thumbnailsPerSprite. When a new sprite is loaded, can't you determine the number of rows and columns of thumbnails in the current sprite and then calculate the thumbnail width and height based on the sprite dimensions?

Also I had to change the use of Process because previously it only allowed a maximum runtime of 60 s. This can be easily exceeded for longer videos.

[lehecht]

@mzur

Does it also work if no preview thumbnails is available and how does it look then?

[mzur]

Looks good :+1:

[lehecht]

@mzur

I'm pretty sure that the thumbnail dimensions can also be inferred from the size of the sprites in the JS.

The thumbnail dimensions are computed during the preview now.

[lehecht]

@mzur I pushed my changes for the hover time on the thumbnail element. It's ready to be reviewed.

Remark: I didn't use the css property min-width for narrow thumbnails, because it would also distort the hover time on the thumbnail. I solved the problem by changing the canvas width and height.

[lehecht]

@mzur My code is ready now. Unfortunately, I couldn't reproduce the first bug. Can you tell me if it's still there?

[lehecht]

@mzur The code is ready to be reviewed.

[lehecht]

@mzur My code is ready to be reviewed again. I added the preload function.

[mzur]

I think the error behavior works as expected now.

I also made changes so the thumbnail is only updated when necessary (instead of any hover event) and fixed the initial display of the thumbnail (without first hover event).

[mzur]

Please implement the ProcessNewVideoTest so it does not actually process any videos or generate thumbnails. You can split the actions (video processing, thumbnail generation) into different methods and mock these methods in the ProcessNewVideoStub of the test. This is how it was done before. In the mocked methods you can record the times for checking and also return fake image data for storage in the test disk.

[lehecht]

@mzur Is the task "Preload the previous and next sprite for instant transitions" finished? And should the throttle function still be implemented?

[mzur]

The preloading is finished. Throttling is no longer required.

[lehecht]

@mzur Is it correct that the thumbnail does not disappear any more (i.e. only hover time bar is shown) if it couldn't be found?

[mzur]

If no thumbnail exists then only the time should be shown, yes (if that's hat you meant).

[lehecht]

@mzur Sorry, I mixed up some thoughts. What I meant was that the time is not displayed if a sprite couldn't be found. The last thumbnail of the last sprite is used for the whole missing sprite. If you go to that missing sprite directly, then only the time is shown.

Here a video of what I mean (sprite_1 does not exist): Bildschirmaufzeichnung vom 06.09.2024, 07:39:39.webm

[lehecht]

Please implement the ProcessNewVideoTest so it does not actually process any videos or generate thumbnails. You can split the actions (video processing, thumbnail generation) into different methods and mock these methods in the ProcessNewVideoStub of the test. This is how it was done before. In the mocked methods you can record the times for checking and also return fake image data for storage in the test disk.

What exactly do you mean by mocking the thumbnail creation? Currently, fake images are created which are then transformed into thumbnails and sprites. The method generateVideoThumbnails() does not any use other methods that could be mocked. So I would need to mock the entire generateVideoThumbnails() method, which would make testing useless.

I could split generateVideoThumbnails(), so that the thumbnail and sprite generation is moved to a new method, that could be mocked. But maybe you had something completely different in mind?

[mzur]

What I meant was that the time is not displayed if a sprite couldn't be found.

That's ok and will be different once v-show is used for the component.

I could split generateVideoThumbnails(), so that the thumbnail and sprite generation is moved to a new method, that could be mocked. But maybe you had something completely different in mind?

This is more about extractImagesfromVideo(), which calls ffmpeg. While this method is mocked in the test, there is an explicit exception in some tests that will actually call ffmpeg. Instead, the call to ffmpeg should be mocked. If these tests are meant to test the actual ffmpeg command, then you can remove the tests. Calling out to ffmpeg is too slow. Instead, test the command manually and once everything works there, never change the command again :wink:

We'll see if similar mocking is required for generateVideoThumbnails() or if the calls to Vips are reasonably fast.

[lehecht]

@mzur

Please implement the ProcessNewVideoTest so it does not actually process any videos

If I use only fake videos, the handler() cannot be tested any more, because I had to mock it to accept the fake videos. Should I delete all the tests for the handler?

[lehecht]

That's ok and will be different once v-show is used for the component.

I'm a bit confused now. Can you explain how the thumbnail preview should behave when v-show is used?

[mzur]

If I use only fake videos, the handler() cannot be tested any more, because I had to mock it to accept the fake videos. Should I delete all the tests for the handler?

Which handler() do you mean? Only mock the methods of the class that do calls to FFmpeg or Vips. If there are methods that do something else, too, split the methods so you have methods that only call FFmpeg or Vips. These can be mocked in the test stub class.

I'm a bit confused now. Can you explain how the thumbnail preview should behave when v-show is used?

It's basically this line. It should be changed to something like:

- v-if="showThumbPreview"
+ v-if="thumbnailEnabledInSettings"
+ v-show="thumbnailVisibleOnMouseOver"

So the if is controlled with the settings switch and the show is controlled by the mouse over event. Then do any changes in the preview thumbnail code that are required to make it work again (as some parts rely on it being destroyed and created each time but now it will be created only once).

[lehecht]

@mzur

Please implement the ProcessNewVideoTest so it does not actually process any videos

Which handler() do you mean? Only mock the methods of the class that do calls to FFmpeg or Vips. If there are methods that do something else, too, split the methods so you have methods that only call FFmpeg or Vips. These can be mocked in the test stub class.

I don't know how to use fake videos together with this handler. The ProcessNewVideosTest class also contains tests just for this handler. But because fake videos are not actual videos, all tests for this handler fail. My questions now is what I should do.

[lehecht]

Then do any changes in the preview thumbnail code that are required to make it work again (as some parts rely on it being destroyed and created each time but now it will be created only once).

If I apply your changes, almost everything seems to work again. Except that the missing sprites are still displayed by the last sprite. And which parts of the code rely on being destroyed and created? I couldn't find any. Or maybe I didn't get, what you wanted to say?

[lehecht]

@mzur My code is ready to be reviewed.