Add file attachment thumbnails for video files.

Review Request #10934 — Created March 2, 2020 and submitted

Information

Review Board
release-4.0.x

Reviewers

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.

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.

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

Description From Last Updated

Col: 39 Missing semicolon.

reviewbotreviewbot

Can we fix the alignment here?

daviddavid

Just a question about this line, Can we always assume that the video will be playing if this section doesn't …

bui1bui1

Tackling onto the other comment, if the video ended up never playing and this_.playingVideo = true i.e if we tried …

bui1bui1
Checks run (1 failed, 1 succeeded)
flake8 passed.
JSHint failed.

JSHint

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

  2. 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

    1. 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.

  3. 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?

    1. 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.

  4. 
      
bui1
  1. Ship It!
  2. 
      
david
  1. 
      
  2. reviewboard/attachments/mimetypes.py (Diff revision 2)
     
     
     
    Show all issues

    Can we fix the alignment here?

    1. 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.

    2. 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.

  3. 
      
chipx86
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (d1c2ddc)