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

Classifier: Custom Video Controls #3684

Merged
merged 41 commits into from
Oct 10, 2022
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
838c5eb
Prepare SingleVideoViewer for adding unit tests
goplayoutside3 Sep 12, 2022
f2e15e5
clean up a bit
goplayoutside3 Sep 12, 2022
5ad95ce
create default storybook for SingleVideoViewerContainer
goplayoutside3 Sep 13, 2022
2f61ab3
working toward handling loading states
goplayoutside3 Sep 13, 2022
6d0d984
more renaming for clarity
goplayoutside3 Sep 13, 2022
0f1feb1
refactor SingleVideoViewerContainer as a functional component
goplayoutside3 Sep 13, 2022
d49c09e
add translation to SubjectViewer and unit tests for SingleVideoViewer…
goplayoutside3 Sep 13, 2022
6735ede
add SingleVideoViewer unit test
goplayoutside3 Sep 13, 2022
7be841c
clean up SingleVideoViewerContainer's readme
goplayoutside3 Sep 13, 2022
9458a80
use ternary for videoSrc in SingleVideoViewerContainer
goplayoutside3 Sep 15, 2022
36c692a
only render the drawing tools interaction layers when enableDrawing i…
goplayoutside3 Sep 15, 2022
fec8ad3
Remove FormattedTime and defaultProps
goplayoutside3 Sep 15, 2022
c5f371a
combine Slider component into VideoController file
goplayoutside3 Sep 15, 2022
610c073
Working toward imporoving VideoController performance
goplayoutside3 Sep 16, 2022
249a303
memoize ReactPlayer for slider performance
goplayoutside3 Sep 16, 2022
96f36b8
clean up formatTimeStamp and its unit tests
goplayoutside3 Sep 16, 2022
720cae4
getFixedNumber unit tests and docs
goplayoutside3 Sep 16, 2022
8b9af19
handle invalid inputs to getFixedNumber() and formatTimeStamp()
goplayoutside3 Sep 16, 2022
d9e5729
handle playback rate label
goplayoutside3 Sep 16, 2022
e8cc4d6
add unit tests for VideoController
goplayoutside3 Sep 19, 2022
5d62695
create custom theme for VideoController
goplayoutside3 Sep 19, 2022
f8454ae
add volume and fullscreen controls
goplayoutside3 Sep 19, 2022
73dd3c0
build custom volume range input
goplayoutside3 Sep 20, 2022
49261f6
fix grid styling of video controls
goplayoutside3 Sep 20, 2022
1366596
clean up and add more unit tests
goplayoutside3 Sep 20, 2022
50d9211
Merge branch 'master' into single-video-viewer-upgrade
goplayoutside3 Sep 20, 2022
e7d5caf
Merge branch 'single-video-viewer-upgrade'
goplayoutside3 Sep 20, 2022
06b7480
fix typo
goplayoutside3 Sep 20, 2022
940f707
Merge branch 'master'
goplayoutside3 Sep 22, 2022
33d0fc0
clean up merge from 'master'
goplayoutside3 Sep 22, 2022
16ded41
Merge branch 'master'
goplayoutside3 Sep 26, 2022
0e32db1
fix missing import
goplayoutside3 Sep 26, 2022
6652812
Merge branch 'master' into video-controller-refactor
goplayoutside3 Oct 10, 2022
afc2bfe
fix small typo in SingleVideoViewer's README
goplayoutside3 Oct 10, 2022
c8be69a
implement change suggestions from PR review
goplayoutside3 Oct 10, 2022
a5e3544
Update README of formatTimeStamp()
goplayoutside3 Oct 10, 2022
f6902d7
update grammar of formatTimeStamp() README
goplayoutside3 Oct 10, 2022
47420a0
add ADR 45 + custom controls only display with drawing tools enabled
goplayoutside3 Oct 10, 2022
134c203
add more stories and unit tests to SingleVideoViewerContainer
goplayoutside3 Oct 10, 2022
ae7d50f
Resolve package.json conflict with master
goplayoutside3 Oct 10, 2022
191fcad
Merge branch 'master' into video-controller-refactor
goplayoutside3 Oct 10, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ The only allowed video media type is mp4.

## Features

SingleVideoViewerContainer handles state for SingleVideoViewer and VideoController. It's also contains an svg and InteractionLayer for drawing on a video subject.
SingleVideoViewerContainer handles state for the `react-player` component and VideoController. It's also contains an svg and InteractionLayer for drawing on a video subject.
goplayoutside3 marked this conversation as resolved.
Show resolved Hide resolved

Refs
- `interactionLayerSVG`: Reference to svg element displayed on top of video subject. Needed for drawing tools' InteractionLayer.
Expand All @@ -24,12 +24,14 @@ Props
State Variables
- `clientWidth`: (number) Returned from `getBoundingClientRect()` on the <video> element in `react-player`.
- `duration`: (number) Duration of the video subject. Seconds rounded to 3 decimal places.
- `fullscreen`: (boolean) Whether or not the video is displayed fullscreen.
- `isPlaying`: (boolean) Whether or not the video subject is playing.
- `isSeeking`: (boolean) Whether or not a user is interacting with the VideoController > Slider.
- `playbackRate`: (number) 1, 0.5, or 0.25 ratio determines the speed of video playback.
Copy link
Member

Choose a reason for hiding this comment

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

[documentation] Does this need to be updated to (string) '1x', '0.5x', or '0.25x'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally, I'll update the README.

- `timeStamp`: (number) Current played timestamp of video subject.
- `timeStamp`: (number) Represented by percent of subject played (0 to 1).
- `videoHeight`: (number) Natural height of video subject file.
- `videoWidth`: (number) Natural width of the video subject file.
- `volume`: (number) Number between 0 and 1. It's passed to the react-player to control volume.
- `volumeOpen`: (boolean) Determines whether the VideoController's volume range input is displayed or not.

## External Setup: Workflows and Subjects

Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
import PropTypes from 'prop-types'
import styled from 'styled-components'
import { Box } from 'grommet'
import React, { useState, useRef } from 'react'
import React, { useMemo, useState, useRef } from 'react'
import { useTranslation } from 'react-i18next'
import asyncStates from '@zooniverse/async-states'
import ReactPlayer from 'react-player'

import SVGContext from '@plugins/drawingTools/shared/SVGContext'

import getFixedNumber from '../../helpers/getFixedNumber'
import InteractionLayer from '../InteractionLayer'
import locationValidator from '../../helpers/locationValidator'
import SingleVideoViewer from './SingleVideoViewer'
import VideoController from '../VideoController/VideoController'
import getFixedNumber from '../../helpers/getFixedNumber'
import VideoController from './components/VideoController'

const SubjectContainer = styled.div`
position: relative;
Expand All @@ -34,12 +34,14 @@ function SingleVideoViewerContainer({
}) {
const [clientWidth, setClientWidth] = useState(0)
const [duration, setDuration] = useState(0)
const [fullscreen, setFullscreen] = useState(false)
const [isPlaying, setIsPlaying] = useState(false)
const [isSeeking, setIsSeeking] = useState(false)
const [playbackRate, setPlaybackRate] = useState(1)
const [playbackRate, setPlaybackRate] = useState('1x')
Copy link
Member

Choose a reason for hiding this comment

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

[minor] This probably comes down to subjective preferences, but I like storing "pure" values in states. So I'd...

  • keep playbackRate as a number (1) instead of a string ('1x')
  • and put that const sanitizedRate = Number(playbackRate.slice(0, -1)) string-to-number transformation into handleSpeedChange() instead

Again though, this is just a matter of preference. As long as it works and mostly makes sense, it's fine. 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. At the time, I did this so the VIdeoController's speed control's labels will match that of PFE. I like your suggestion to store a pure value in state and then render a string label such as 0.5x. I'll leave it as is, and create a to-do in the Video Annotator project board to refactor while I add simple speed control buttons intended to display with the native browser video controls in non-drawing-tools projects.

const [timeStamp, setTimeStamp] = useState(0)
const [videoHeight, setVideoHeight] = useState(0)
const [videoWidth, setVideoWidth] = useState(0)
const [volume, setVolume] = useState(1)
const [volumeOpen, toggleVolumeOpen] = useState(false)

const interactionLayerSVG = useRef()
const playerRef = useRef()
Expand Down Expand Up @@ -75,17 +77,14 @@ function SingleVideoViewerContainer({
}
}

const enableDrawing = loadingState === asyncStates.success && enableInteractionLayer
const enableDrawing = enableInteractionLayer && loadingState === asyncStates.success

/* ==================== SingleVideoViewer react-player ==================== */
/* ==================== react-player ==================== */

const handleVideoProgress = reactPlayerState => {
const { played } = reactPlayerState
const fixedNumber = getFixedNumber(played, 5)
// TO DO: Why wouldn't you set timestamp when seeking?
if (!isSeeking) {
setTimeStamp(fixedNumber)
}
const { played } = reactPlayerState // percentage of video played (0 to 1)
const fixedNumber = getFixedNumber(played, 3)
setTimeStamp(fixedNumber)
}

const handleVideoDuration = duration => {
Expand All @@ -105,25 +104,75 @@ function SingleVideoViewerContainer({
setPlaybackRate(rate)
}

const handleSliderMouseUp = () => {
setIsSeeking(false)
const handleSliderChange = e => {
const newTimeStamp = e.target.value
playerRef?.current.seekTo(newTimeStamp, 'seconds')
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be current?., or will the existence of playerRef always ensure there's a current object?

Copy link
Contributor

Choose a reason for hiding this comment

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

playerRef.current will be null when the component mounts. I think the ref will be set after the first render. Probably safe here, if this is a handler that’s called once the player DOM exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

handleSliderChange() is used in VideoController. VideoController renders independently of playerRef, so I'll add current? here.

}

const handleSliderMouseDown = () => {
setIsPlaying(false)
setIsSeeking(true)
const handleVolume = e => {
setVolume(e.target.value)
}

/* When VideoController > Slider is clicked or scrubbed */
const handleSliderChange = e => {
const played = getFixedNumber(e.target.value, 5)
playerRef?.current.seekTo(played)
const handleVolumeOpen = () => {
const prevVolumeOpen = volumeOpen
toggleVolumeOpen(!prevVolumeOpen)
}

const handleFullscreen = () => {
if (fullscreen) {
try {
document.exitFullscreen()
setFullscreen(false)
} catch (error) {
console.log(error)
}
} else {
try {
playerRef.current?.getInternalPlayer().requestFullscreen()
setFullscreen(true)
} catch (error) {
console.log(error)
}
}
}

const handlePlayerError = (error) => {
onError(error)
}

const sanitizedRate = Number(playbackRate.slice(0, -1))

/* Memoized so onProgress() and setTimeStamp() don't trigger each other */
const memoizedViewer = useMemo(() => (
<ReactPlayer
controls={false}
height='100%'
onDuration={handleVideoDuration}
onEnded={handleVideoEnded}
onError={handlePlayerError}
onReady={onReactPlayerReady}
onProgress={handleVideoProgress}
playing={isPlaying}
playbackRate={sanitizedRate}
progressInterval={100} // milliseconds
ref={playerRef}
width='100%'
volume={volume}
url={videoSrc}
config={{
file: { // styling the <video> element
attributes: {
style: {
display: 'block',
height: '100%',
width: '100%'
}
}
}
}}
/>
), [isPlaying, playbackRate, videoSrc, volume])

const canvas = transformLayer?.current
const interactionLayerScale = clientWidth / videoWidth

Expand All @@ -132,17 +181,7 @@ function SingleVideoViewerContainer({
{videoSrc
? (
<SubjectContainer>
<SingleVideoViewer
isPlaying={isPlaying}
onDuration={handleVideoDuration}
onEnded={handleVideoEnded}
onError={handlePlayerError}
onReactPlayerReady={onReactPlayerReady}
onProgress={handleVideoProgress}
playbackRate={playbackRate}
playerRef={playerRef}
url={videoSrc}
/>
{memoizedViewer}
{enableDrawing && (
<DrawingLayer>
<Box overflow='hidden'>
Expand Down Expand Up @@ -175,15 +214,18 @@ function SingleVideoViewerContainer({
<Box>{t('SubjectViewer.SingleVideoViewerContainer.error')}</Box>
)}
<VideoController
isPlaying={isPlaying}
played={timeStamp}
playbackRate={playbackRate}
duration={duration}
isPlaying={isPlaying}
handleFullscreen={handleFullscreen}
handleVolumeOpen={handleVolumeOpen}
onPlayPause={handlePlayPause}
onSpeedChange={handleSpeedChange}
onSliderMouseUp={handleSliderMouseUp}
onSliderMouseDown={handleSliderMouseDown}
onSliderChange={handleSliderChange}
onSpeedChange={handleSpeedChange}
onVolumeChange={handleVolume}
playbackRate={playbackRate}
timeStamp={timeStamp}
volume={volume}
volumeOpen={volumeOpen}
/>
</>
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { render, screen, waitFor } from '@testing-library/react'
import { expect } from 'chai'
import { Factory } from 'rosie'
import { Provider } from 'mobx-react'
import { Grommet } from 'grommet'
import zooTheme from '@zooniverse/grommet-theme'

import mockStore from '@test/mockStore'
import SingleVideoViewerContainer from './SingleVideoViewerContainer'
Expand All @@ -12,7 +14,8 @@ describe('Component > SingleVideoViewerContainer', function () {
const mockSubject = Factory.build('subject', {
locations: [
{
'video/mp4': 'https://panoptes-uploads.zooniverse.org/subject_location/239f17f7-acf9-49f1-9873-266a80d29c33.mp4'
'video/mp4':
'https://panoptes-uploads.zooniverse.org/subject_location/239f17f7-acf9-49f1-9873-266a80d29c33.mp4'
}
]
})
Expand All @@ -24,9 +27,9 @@ describe('Component > SingleVideoViewerContainer', function () {
it('should render a video html element with subject url as src', async function () {
const { container } = render(
<Provider classifierStore={store}>
<SingleVideoViewerContainer
subject={mockSubject}
/>
<Grommet theme={zooTheme}>
<SingleVideoViewerContainer subject={mockSubject} />
</Grommet>
</Provider>
)
// We need to wait for React Player to be ready
Expand All @@ -49,11 +52,15 @@ describe('Component > SingleVideoViewerContainer', function () {
it('should display an error message and no video html element ', function () {
const { container } = render(
<Provider classifierStore={store}>
<SingleVideoViewerContainer />
<Grommet theme={zooTheme}>
<SingleVideoViewerContainer />
</Grommet>
</Provider>
)
const videoElement = container.querySelector('video')
const errorMessage = screen.getByText('SubjectViewer.SingleVideoViewerContainer.error')
const errorMessage = screen.getByText(
'SubjectViewer.SingleVideoViewerContainer.error'
)
expect(videoElement).to.be.null()
expect(errorMessage).exists()
})
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# VideoController

Used in SingleVideoViewer. Custom video controls are necessary for building drawing tools layers on top of a video subject. The `react-player` used to display a video subject has built-in controls, but their position overlaps some of the video. When an svg is placed on top of the player to record annotations, the built-in controls become unusable, hence the need for custom controls displayed below the subject.
Copy link
Member

Choose a reason for hiding this comment

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

👍 I really like that we not only explain what this component is, but also WHY it exists. 👍


## Features

- Play
- Pause
- Display timestamp
- Scrubbing
- Change playback rate
- Adjust the volume
- Request fullscreen

## Props
- `duration`: (number) Duration of video subject in seconds.
- `isPlaying`: (boolean) State variable in SingleVideoViewerContainer determines if video player is playing.
- `handleFullscreen`: (func) Function passed from SingleVideoViewerContainer and called when fullscreen button is clicked.
- `handleVolumeOpen`: (func) Function passed from SingleVideoViewerContainer and called when volume icon is clicked.
- `onPlayPause`: (func) Function passed from SingleVideoViewerContainer and called when player is played or paused.
- `onSpeedChange`: (func) Function passed from SingleVideoViewerContainer and called when the playback rate Select component is changed.
Copy link
Member

Choose a reason for hiding this comment

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

[documentation] Might be worth noting how the function is called. (See note on onSliderChange)

- `onSliderChange`: (func) Function passed from SingleVideoViewerContainer and called when Slider input is changed.
Copy link
Member

Choose a reason for hiding this comment

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

[documentation] Might be worth noting how the function is called. e.g. When the slider is changed, the function will be called with func(changeEvent), where changeEvent.target.value is the time (number, in seconds) in the video that the user is scrubbing towards/seeking to go to.

Copy link
Contributor

Choose a reason for hiding this comment

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

MDN documents change events for range inputs. Would it be helpful to link to that? I’m not sure if that’s what you’re asking @shaunanoordin? I read your comment as ‘explain how change events work.’

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I lean toward Jim's suggestion. For instance, the variable passed to onSliderChange() is distinctly labeled as e.target.value in SingleVideoViewerContainer.

- `onVolumeChange`: (func) Called when the volume range input is changed.
Copy link
Member

Choose a reason for hiding this comment

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

[documentation] Might be worth noting how the function is called. (See note on onSliderChange)

- `playbackRate`: (number) From 0 to 1.
- `timeStamp`: (number) Measured from 0 to 1. The percentage of the video subject played.
- `volume`: (number) Number between 0 and 1.
- `volumeOpen`: (boolean) Controls whether the volume range input is displayed or not.
Loading