Summary

Add file attachment thumbnails for video files.

Review Request #10934 — Created March 2, 2020 and submitted March 12, 2020, 7:01 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This implements a new VideoMimetype handler for all video/* files,
which provide a browser-supplied thumbnail for uploaded videos. This
handler will generate an HTML thumbnail that sets up a <video> tag
starting 0.5 seconds into the video, prompting the browser to load
enough of the video to display an early frame and to stop. The video is
set to not auto-play, to mute audio, and to preload only metadata along
with the frame.

When hovering over the thumbnail, the RB.FileAttachmentThumbnailView
will look for a <video> tag and play the video on loop, instead of
scrolling the frame. When the mouse moves away, the video will pause,
but retain the current position within the video for later resuming.

This does a lot to help video files not appear so blank. There is room
for improvement. For instance, if the video file cannot be played by the
browser, or it can only play audio but not the video, then the thumbnail
will appear blank. This is no different than it was before. Fixing it
would involve providing a poster="..." argument that points to an
image to display. However, for now, this is being left out, as this is
still an improvement over the existing behavior.

Testing Done

Tested with a bunch of videos (some that could be displayed, some that
could not). Verified that the browsers weren't downloading the entire
thing up-front.

Hovered over the thumbnail and saw it start playing. Moved away and
saw it pause. Hovered over again and saw it resume.

Tested in Chrome, Firefox, Edge, and Safari. Safari did not support
any of this for my test videos, but did not break.

Commits

Summary	ID
Add file attachment thumbnails for video files. This implements a new `VideoMimetype` handler for all `video/*` files, which provide a browser-supplied thumbnail for uploaded videos. This handler will generate an HTML thumbnail that sets up a `<video>` tag starting 0.5 seconds into the video, prompting the browser to load enough of the video to display an early frame and to stop. The video is set to not auto-play, to mute audio, and to preload only metadata along with the frame. When hovering over the thumbnail, the `RB.FileAttachmentThumbnailView` will look for a `<video>` tag and play the video on loop, instead of scrolling the frame. When the mouse moves away, the video will pause, but retain the current position within the video for later resuming. This does a lot to help video files not appear so blank. There is room for improvement. For instance, if the video file cannot be played by the browser, or it can only play audio but not the video, then the thumbnail will appear blank. This is no different than it was before. Fixing it would involve providing a `poster="..."` argument that points to an image to display. However, for now, this is being left out, as this is still an improvement over the existing behavior.	cb769b722d88197aec23f9da2c5989a456a59fd9

Summary

Add file attachment thumbnails for video files.

This implements a new `VideoMimetype` handler for all `video/*` files, which provide a browser-supplied thumbnail for uploaded videos. This handler will generate an HTML thumbnail that sets up a `<video>` tag starting 0.5 seconds into the video, prompting the browser to load enough of the video to display an early frame and to stop. The video is set to not auto-play, to mute audio, and to preload only metadata along with the frame. When hovering over the thumbnail, the `RB.FileAttachmentThumbnailView` will look for a `<video>` tag and play the video on loop, instead of scrolling the frame. When the mouse moves away, the video will pause, but retain the current position within the video for later resuming. This does a lot to help video files not appear so blank. There is room for improvement. For instance, if the video file cannot be played by the browser, or it can only play audio but not the video, then the thumbnail will appear blank. This is no different than it was before. Fixing it would involve providing a `poster="..."` argument that points to an image to display. However, for now, this is being left out, as this is still an improvement over the existing behavior.

cb769b722d88197aec23f9da2c5989a456a59fd9

Files

Issues

Description	From	Last Updated
Col: 39 Missing semicolon.	reviewbot	March 2, 2020, 9:16 p.m.
Can we fix the alignment here?	david	March 12, 2020, 6:59 p.m.
Just a question about this line, Can we always assume that the video will be playing if this section doesn't …	bui1	March 3, 2020, 2:38 p.m.
Tackling onto the other comment, if the video ended up never playing and this_.playingVideo = true i.e if we tried …	bui1	March 3, 2020, 2:38 p.m.

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/fileAttachmentThumbnailView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 39
 Missing semicolon.
```

Change Summary:

Fixed a missing semicolon.

Commits:

	Summary	ID
	Add file attachment thumbnails for video files. This implements a new `VideoMimetype` handler for all `video/*` files, which provide a browser-supplied thumbnail for uploaded videos. This handler will generate an HTML thumbnail that sets up a `<video>` tag starting 0.5 seconds into the video, prompting the browser to load enough of the video to display an early frame and to stop. The video is set to not auto-play, to mute audio, and to preload only metadata along with the frame. When hovering over the thumbnail, the `RB.FileAttachmentThumbnailView` will look for a `<video>` tag and play the video on loop, instead of scrolling the frame. When the mouse moves away, the video will pause, but retain the current position within the video for later resuming. This does a lot to help video files not appear so blank. There is room for improvement. For instance, if the video file cannot be played by the browser, or it can only play audio but not the video, then the thumbnail will appear blank. This is no different than it was before. Fixing it would involve providing a `poster="..."` argument that points to an image to display. However, for now, this is being left out, as this is still an improvement over the existing behavior.	fe2ef00a0ad815a90e9545b4d25a7b150bb2ed9c
	Add file attachment thumbnails for video files. This implements a new `VideoMimetype` handler for all `video/*` files, which provide a browser-supplied thumbnail for uploaded videos. This handler will generate an HTML thumbnail that sets up a `<video>` tag starting 0.5 seconds into the video, prompting the browser to load enough of the video to display an early frame and to stop. The video is set to not auto-play, to mute audio, and to preload only metadata along with the frame. When hovering over the thumbnail, the `RB.FileAttachmentThumbnailView` will look for a `<video>` tag and play the video on loop, instead of scrolling the frame. When the mouse moves away, the video will pause, but retain the current position within the video for later resuming. This does a lot to help video files not appear so blank. There is room for improvement. For instance, if the video file cannot be played by the browser, or it can only play audio but not the video, then the thumbnail will appear blank. This is no different than it was before. Fixing it would involve providing a `poster="..."` argument that points to an image to display. However, for now, this is being left out, as this is still an improvement over the existing behavior.	a3ecb12c8f07174b21ddb248e6f3e61530815b84

Diff:

Revision 2 (+180 -52)

Show changes

	reviewboard/attachments/__init__.py
	reviewboard/attachments/mimetypes.py
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The code itself looks very good! Just had a couple questions.

reviewboard/static/rb/js/views/fileAttachmentThumbnailView.es6.js (Diff revision 2)

The issue has been dropped. Show all issues

Just a question about this line,

Can we always assume that the video will be playing if this section doesn't return a promise?
What happens if we set this._playingVideo = true; but the video itself isn't playing? Then it would give a false positive

chipx86 March 3, 2020, 2:03 p.m.

Good question! It would, and this might be a problem if we were doing anything more advanced, but we're just using this to control whether we need to try to pause a video when moving the mouse outside the thumbnail. If the video isn't actually playing, that's fine -- our call to pause() will just do nothing.

Potentially, in the lack-of-promises case, we could look at events to determine if it's playing, but it's not really worth the extra setup, since our needs are so basic.

reviewboard/static/rb/js/views/fileAttachmentThumbnailView.es6.js (Diff revision 2)

The issue has been dropped. Show all issues

Tackling onto the other comment, if the video ended up never playing and this_.playingVideo = true i.e if we tried to pause an already paused video, is there any implications to that?

chipx86 March 3, 2020, 2:03 p.m.

There doesn't seem to be. The documentation on this method reads that pause() will just do nothing if the video isn't playing. I've tried with a file that couldn't be successfully played, and pausing didn't break anything there.

Ship it!

```
Ship It!
```

Ship it!

reviewboard/attachments/mimetypes.py (Diff revision 2)

The issue has been resolved. Show all issues

Can we fix the alignment here?

chipx86 March 9, 2020, 3:43 a.m.

There's no actual newline here. Avoiding adding the unnecessary spaces. This follows the approach of other templates in here. That said, I could just change it to output newlines, but it's still probably not worth it for the output.

david March 11, 2020, 1:03 p.m.

I don't think avoiding the unnecessary spaces is really something valuable here, and I'd prioritize readability. It's all gzipped on its way down to the client anyway.

Change Summary:

Added visual alignment for the video thumbnail template.

Commits:

	Summary	ID
	Add file attachment thumbnails for video files. This implements a new `VideoMimetype` handler for all `video/*` files, which provide a browser-supplied thumbnail for uploaded videos. This handler will generate an HTML thumbnail that sets up a `<video>` tag starting 0.5 seconds into the video, prompting the browser to load enough of the video to display an early frame and to stop. The video is set to not auto-play, to mute audio, and to preload only metadata along with the frame. When hovering over the thumbnail, the `RB.FileAttachmentThumbnailView` will look for a `<video>` tag and play the video on loop, instead of scrolling the frame. When the mouse moves away, the video will pause, but retain the current position within the video for later resuming. This does a lot to help video files not appear so blank. There is room for improvement. For instance, if the video file cannot be played by the browser, or it can only play audio but not the video, then the thumbnail will appear blank. This is no different than it was before. Fixing it would involve providing a `poster="..."` argument that points to an image to display. However, for now, this is being left out, as this is still an improvement over the existing behavior.	a3ecb12c8f07174b21ddb248e6f3e61530815b84
	Add file attachment thumbnails for video files. This implements a new `VideoMimetype` handler for all `video/*` files, which provide a browser-supplied thumbnail for uploaded videos. This handler will generate an HTML thumbnail that sets up a `<video>` tag starting 0.5 seconds into the video, prompting the browser to load enough of the video to display an early frame and to stop. The video is set to not auto-play, to mute audio, and to preload only metadata along with the frame. When hovering over the thumbnail, the `RB.FileAttachmentThumbnailView` will look for a `<video>` tag and play the video on loop, instead of scrolling the frame. When the mouse moves away, the video will pause, but retain the current position within the video for later resuming. This does a lot to help video files not appear so blank. There is room for improvement. For instance, if the video file cannot be played by the browser, or it can only play audio but not the video, then the thumbnail will appear blank. This is no different than it was before. Fixing it would involve providing a `poster="..."` argument that points to an image to display. However, for now, this is being left out, as this is still an improvement over the existing behavior.	cb769b722d88197aec23f9da2c5989a456a59fd9

Diff:

Revision 3 (+180 -52)

Show changes

	reviewboard/attachments/__init__.py
	reviewboard/attachments/mimetypes.py
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Status:: Completed
Change Summary:: Pushed to release-4.0.x (d1c2ddc)