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

[Videos] Video player - PR #1 - basic player #4731

Merged
merged 34 commits into from
Jul 25, 2024
Merged

Conversation

mozzius
Copy link
Member

@mozzius mozzius commented Jul 4, 2024

Stacked on #4638

Basic video player. Other stuff is stacked on it, probably don't merge until those are ready

Copy link

render bot commented Jul 4, 2024

@mozzius mozzius changed the title [Videos] Video player - PR #1 [Videos] Video player - PR #1 - basic player Jul 4, 2024
Copy link

github-actions bot commented Jul 4, 2024

The Pull Request introduced fingerprint changes against the base commit:

Fingerprint diff
[{"type":"file","filePath":"package.json","reasons":["expoConfigPlugins"],"hash":"0f3b60558eddc1ed3905fe647b04f853cf1612bb"},{"type":"dir","filePath":"node_modules/expo-video/android","reasons":["expoAutolinkingAndroid"],"hash":"457855e16ebdbf23264e26af22dafdbaae35b0e8"},{"type":"dir","filePath":"patches","reasons":["patchPackage"],"hash":"229ffcdfaa62ce8cfdfe434d1f8354f0d34d20c9"}]

Generated by PR labeler 🤖

Copy link

github-actions bot commented Jul 4, 2024

Old size New size Diff
6.63 MB 7.01 MB 386.59 KB (5.70%)

@mozzius mozzius changed the base branch from main to samuel/videos-compressor July 4, 2024 20:41
@mozzius mozzius changed the base branch from samuel/videos-compressor to main July 25, 2024 15:18
Copy link
Contributor

@haileyok haileyok left a comment

Choose a reason for hiding this comment

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

Quick glance this looks good. Gonna be tweaking it over time, so didn't super thoroughly test it.

}
}, [])

useFrameCallback(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to be using this at all on web? I assume not with the intersection observer route it sounds like we're going to take.

I think I'm going to be using this a little in this implementation I cooked up on paper, but its fine to leave this logic here for now (or just empty this out, doesn't really matter)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, impl on web will be completely different


import {atoms as a} from '#/alf'

export const VideoEmbedInner = ({source}: {source: string}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

not reviewing this because i think we address it on part 2, correct me if wrong tho

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct!

@@ -348,6 +349,7 @@ let FeedItemInner = ({
postAuthor={post.author}
onOpenEmbed={onOpenEmbed}
/>
<VideoEmbed source="https://vid.jazco.dev/watch/did:plc:q6gjnaw2blty4crticxkmujt/QmVdiZ6Bw6rTD98NTJzFWdmCTnWgiVcR7qxt127X5Km1JG/playlist.m3u8" />
Copy link
Contributor

Choose a reason for hiding this comment

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

remember to remove this line before merge

Copy link
Contributor

Choose a reason for hiding this comment

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

or gate :)

@haileyok haileyok marked this pull request as ready for review July 25, 2024 19:01
@mozzius mozzius merged commit 00240b9 into main Jul 25, 2024
6 checks passed
@mozzius mozzius deleted the samuel/videos-player branch July 25, 2024 19:41
@efstathiosntonas
Copy link

efstathiosntonas commented Jul 31, 2024

@mozzius Hi, did you encounter this on component unmount while testing it?

[Reanimated] The view with tag XXXX returned an invalid measurement response. Please make sure the view is currently rendered.

I tried to use the useFrameCallback on my (mobile) app and I've encountered the warning above. Note that while getting this warning it works fine in development mode, on production it will probably crash.

While using this though the error went away (and functionality remains the same):

    const checkInView = useCallback(() => {
      'worklet';
      if (!aref) return;

      const measurement = measure(aref);
      if (measurement === null) {
        return;
      }
      if (
        measurement.pageY >= 0 &&
        measurement.pageY + measurement.height <= windowHeight
      ) {
        if (hasLeftView.value) {
          runOnJS(onEnterView)();
          hasLeftView.value = false;
        }
      } else {
        if (!hasLeftView.value) {
          runOnJS(onLeaveView)();
          hasLeftView.value = true;
        }
      }
    }, [aref, hasLeftView, onEnterView, onLeaveView, windowHeight]);

    useEffect(() => {
      const interval = setInterval(() => {
        runOnUI(checkInView)();
      }, 500);

      return () => clearInterval(interval);
    }, [checkInView]);

I believe this is ligther too since it calls every 500ms instead on every single frame.

@haileyok
Copy link
Contributor

@efstathiosntonas We're not actually using that anyway in favor of a better solution (thankfully!) There is some context on why the interval trick doesn't work in all cases and why useFrameCallback is the hack we landed on (and have been using for a while) in

const frameCallback = useFrameCallback(() => {

If it's useful I don't mind sharing, gotta dig up the PR where I added it.

estrattonbailey added a commit that referenced this pull request Jul 31, 2024
* origin/main: (30 commits)
  Only show "followed you back" when appropriate (#4849)
  [Web] Retrigger onEndReached if needed when content height changes (#4859)
  Remove unused NoopFeedTuner (#4856)
  useDedupe callback (#4855)
  [Video] Uploads (#4754)
  Make label required in link components (#4844)
  Boolean filter improvement alternative: TS upgrade (#4840)
  Add label to profile card (#4843)
  Improve a11y on noty feed (#4842)
  Add labels in feed card (#4836)
  Add labels to mod details dialog (#4839)
  Add labels to a few missing places (#4838)
  Add labels in list card (#4837)
  Refactor feed slices (#4834)
  `true` (#4833)
  Replace `import hairlineWidth =` with const (#4831)
  [Videos] Video player - PR #1 - basic player (#4731)
  Bump 1.90 (#4832)
  Fix sloppy filter(Boolean) types (#4830)
  Fuggedaboudit (#4829)
  ...
estrattonbailey added a commit that referenced this pull request Jul 31, 2024
* origin/main: (37 commits)
  Only show "followed you back" when appropriate (#4849)
  [Web] Retrigger onEndReached if needed when content height changes (#4859)
  Remove unused NoopFeedTuner (#4856)
  useDedupe callback (#4855)
  [Video] Uploads (#4754)
  Make label required in link components (#4844)
  Boolean filter improvement alternative: TS upgrade (#4840)
  Add label to profile card (#4843)
  Improve a11y on noty feed (#4842)
  Add labels in feed card (#4836)
  Add labels to mod details dialog (#4839)
  Add labels to a few missing places (#4838)
  Add labels in list card (#4837)
  Refactor feed slices (#4834)
  `true` (#4833)
  Replace `import hairlineWidth =` with const (#4831)
  [Videos] Video player - PR #1 - basic player (#4731)
  Bump 1.90 (#4832)
  Fix sloppy filter(Boolean) types (#4830)
  Fuggedaboudit (#4829)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants