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

[$500] Changing playback speed is not taking effect when changing playback speed before playing #36827

Closed
3 of 6 tasks
kavimuru opened this issue Feb 20, 2024 · 34 comments
Closed
3 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Feb 20, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 1.4.43-0
Reproducible in staging?: y
Reproducible in production?: new feature
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: applause internal team
Slack conversation:

Action Performed:

  1. Open a chat
  2. Click on '+' icon > Add attachment > select a video
  3. Click on three dot icon > Playback speed > select 2 or other option
  4. Click on play icon

Expected Result:

Change in playback speed setting should take effect

Actual Result:

Change in playback seep doesn't take effect

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6385212_1708392957244.Screen_Recording_2024-02-16_at_4.09.36_in_the_afternoon.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~017f68d54c34c3a131
  • Upwork Job ID: 1759791176103968768
  • Last Price Increase: 2024-02-20
  • Automatic offers:
    • dukenv0307 | Contributor | 0
    • shubham1206agra | Contributor | 0
@kavimuru kavimuru added DeployBlockerCash This issue or pull request should block deployment External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 20, 2024
@melvin-bot melvin-bot bot changed the title Changing playback speed is not taking effect when changing playback speed before playing [$500] Changing playback speed is not taking effect when changing playback speed before playing Feb 20, 2024
Copy link

melvin-bot bot commented Feb 20, 2024

Job added to Upwork: https://www.upwork.com/jobs/~017f68d54c34c3a131

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 20, 2024
Copy link

melvin-bot bot commented Feb 20, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

Copy link

melvin-bot bot commented Feb 20, 2024

Triggered auto assignment to @miljakljajic (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Feb 20, 2024
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Feb 20, 2024

Triggered auto assignment to @MariaHCD (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@kavimuru
Copy link
Author

We think this bug might be related to #vip-vsb
cc @quinthar

@dukenv0307
Copy link
Contributor

dukenv0307 commented Feb 20, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Change in playback seep doesn't take effect

What is the root cause of that problem?

We don't pass rate prop into Video component to control the speed of the video so the speed is always the default

Ref: https://docs.expo.dev/versions/latest/sdk/video/#rate

What changes do you think we should make in order to solve the problem?

Return currentPlaybackSpeed in contextValue of VideoPlayerContexts

const contextValue = useMemo(() => ({menuItems, updatePlaybackSpeed, currentPlaybackSpeed}), [menuItems, updatePlaybackSpeed]);

const contextValue = useMemo(() => ({menuItems, updatePlaybackSpeed}), [menuItems, updatePlaybackSpeed]);

const {currentPlaybackSpeed} = useVideoPopoverMenuContext();


rate={currentPlaybackSpeed}

Then in BaseVideoPlayer get this speed and pass it as rate prop into Video component.

What alternative solutions did you explore? (Optional)

Each video should have a default speed is 1.0, we can create a speed state in BaseVideoPlayer and then when we update the speed we will update speed state instead of updating it in VideoPlayerContexts

Result

Screen.Recording.2024-02-20.at.11.35.23.mov

@MariaHCD
Copy link
Contributor

Looks like this is also related to #30829

cc: @Skalakid @francoisl @akinwale

I think @dukenv0307's proposal makes sense. Thoughts, @Santhosh-Sellavel?

@miljakljajic
Copy link
Contributor

Waiting for feedback from @Santhosh-Sellavel

@shubham1206agra
Copy link
Contributor

@Santhosh-Sellavel is OOO. Since this is a deploy blocker, I will help out here.

@dukenv0307's proposal looks good to me.
🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Feb 20, 2024

Current assignee @MariaHCD is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@shubham1206agra
Copy link
Contributor

@MariaHCD
Copy link
Contributor

Thanks, @shubham1206agra!

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 20, 2024
Copy link

melvin-bot bot commented Feb 20, 2024

📣 @shubham1206agra 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@MariaHCD
Copy link
Contributor

Although perhaps we shouldn't proceed with a fix for this just yet: https://expensify.slack.com/archives/C01GTK53T8Q/p1708443654362909?thread_ts=1708443084.946829&cid=C01GTK53T8Q

@shubham1206agra
Copy link
Contributor

Yes.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Hourly KSv2 labels Feb 20, 2024
@dukenv0307
Copy link
Contributor

@shubham1206agra The PR is ready for review.

@francoisl
Copy link
Contributor

I don't think we're going to end up reverting the PR, we already merged other fixes and there would be too many conflicts to resolve at this point. @shubham1206agra please proceed with reviewing and testing the PR. Thanks!

@puneetlath
Copy link
Contributor

Removing the deploy blocker label. Seems like something we can fix at the normal cadence.

@puneetlath puneetlath removed the DeployBlockerCash This issue or pull request should block deployment label Feb 29, 2024
@shubham1206agra
Copy link
Contributor

PR was already merged and deployed.

@miljakljajic
Copy link
Contributor

is payment due here?

@shubham1206agra
Copy link
Contributor

Yes

@shubham1206agra
Copy link
Contributor

@miljakljajic Can you start payment process here?

@miljakljajic
Copy link
Contributor

@shubham1206agra and @dukenv0307 sent contracts to you both, previous job expired.

@shubham1206agra
Copy link
Contributor

@miljakljajic I have discussed this internally here. You may close this issue whenever you pay the other contributor as I am keeping track of payment internally and will ask to pay once the issue is resolved. Just write in the payment summary that I still need to be paid.

@miljakljajic
Copy link
Contributor

Thank you @shubham1206agra

@shubham1206agra - owed 500 USD for their work on this issue.

@dukenv0307 please accept the offer when you can.

@dukenv0307
Copy link
Contributor

@dukenv0307 please accept the offer when you can.

@miljakljajic I accepted, thank you!

@shubham1206agra
Copy link
Contributor

@miljakljajic You can process payment here now.

@shubham1206agra
Copy link
Contributor

@miljakljajic Bump here.

@strepanier03 strepanier03 reopened this May 8, 2024
@shubham1206agra
Copy link
Contributor

@miljakljajic Bump for the payment.

@shubham1206agra
Copy link
Contributor

@miljakljajic Bump here.

@miljakljajic
Copy link
Contributor

Paid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants