Page MenuHomePhabricator

[native] VideoPlaybackModal support for encrypted video
ClosedPublic

Authored by bartek on Mar 28 2023, 11:17 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 14, 3:56 PM
Unknown Object (File)
Fri, Apr 12, 4:26 PM
Unknown Object (File)
Wed, Apr 10, 7:52 AM
Unknown Object (File)
Thu, Mar 28, 7:36 AM
Unknown Object (File)
Thu, Mar 28, 7:36 AM
Unknown Object (File)
Thu, Mar 28, 7:36 AM
Unknown Object (File)
Thu, Mar 28, 7:36 AM
Unknown Object (File)
Thu, Mar 28, 7:35 AM
Subscribers

Details

Summary

Very similar to D7225 but for video. For encrypted components, the videoUri is replaced after decryption and then the actual <Video> component is displayed. Caching layer will be added later

According to react-native-video documentation:

Rendering the player component with a null source will init the player, and start playing once a source value is provided.

However, providing null source throws a warn.

Depends on D7224
Depends on D7168

Test Plan

Provided a fake encrypted mediaInfo prop, opened the modal and ensured that a spinner is displayed while decryption is in progress and controls are hidded. After successful decryption, the video starts playing.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Mar 28 2023, 12:04 PM
native/media/video-playback-modal.react.js
673 ↗(On Diff #24303)

Nit: can we avoid inline logic in JSX? I know this is a common pattern outside of the Comm codebase, but I personally don't like it... I think it hurts readability by potentially hiding control flow and increasing indentation levels. I prefer for JSX to be a "leaf" in the AST

686 ↗(On Diff #24303)

For instance, this condition could be moved to line 606, eg.:

let controls;
if (videoSource) {
  controls = (
    ...
  );
}

This makes it really clear to the person reading the controls block that it is not rendered if there is no videoSource

native/media/video-playback-modal.react.js
673 ↗(On Diff #24303)

Sure! When I think about it, your concern makes much sense! I think I might even add this rule to my "personal patterns" ;)

686 ↗(On Diff #24303)

Agreed, will do

Provided a fake encrypted mediaInfo prop, opened the modal and ensured that a spinner is displayed while decryption is in progress and controls are hidded. After successful decryption, the video starts playing.

Wow, this is coming along quickly. Please make sure to address @ashoat's feedback before landing.

native/media/video-playback-modal.react.js
81–114 ↗(On Diff #24303)

Wonder if it'd make sense to pull this out to a hook? Just a thought, can land this diff as-is.

This revision is now accepted and ready to land.Mar 28 2023, 4:30 PM
native/media/video-playback-modal.react.js
81–114 ↗(On Diff #24303)

I was thinking about the same thing. Tried to do that initially, but this hook would require custom dependency array (for image component) and that conflicted with eslint: https://stackoverflow.com/questions/56262515/how-to-handle-dependencies-array-for-custom-hooks-in-react

Finally, I postponed doing this, but will create a task for follow-ups before landing

Address feedback - move logic out of JSX tree