Skip to content
This repository has been archived by the owner on Oct 4, 2023. It is now read-only.

[C-1482] Fix visualizer on mp3 stream #2259

Merged
merged 4 commits into from
Nov 16, 2022
Merged

[C-1482] Fix visualizer on mp3 stream #2259

merged 4 commits into from
Nov 16, 2022

Conversation

raymondjacobson
Copy link
Member

Description

Also fixes some behavior around fadeIn/fadeOut that has been bugging me for a while.
I'm not entirely sure how this fixes it, but my guess is due to the 302 and us not handling the CORS properly.

Dragons

Is there anything the reviewer should be on the lookout for? Are there any dangerous changes?

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.

Locally vs. staging.

I suggest that we cherry pick this onto the release branch after it goes in and re-enable mp3 stream when this is out.

Please verify my changes!

How will this change be monitored?

For features that are critical or could fail silently please describe the monitoring/alerting being added.

Feature Flags

Are all new features properly feature flagged? Describe added feature flags.

@raymondjacobson raymondjacobson marked this pull request as ready for review November 15, 2022 03:18
@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 15, 2022
@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/rj-c-1482

Copy link
Contributor

@dylanjeffers dylanjeffers left a comment

Choose a reason for hiding this comment

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

Noice

Copy link
Contributor

@amendelsohn amendelsohn left a comment

Choose a reason for hiding this comment

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

Wow! Can't believe you got it working so quickly! 👏

packages/web/src/services/audio-player/AudioPlayer.ts Outdated Show resolved Hide resolved
Comment on lines 377 to 379
if (this.gainNode) {
this.gainNode.gain.setValueAtTime(1, 0)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Disclaimer: I still don't fully understand the purpose of this call, so feel free to ignore this suggestion...

All the examples I've seen are using currentTime to set the gain.

To me this makes more sense, since in the other place we call setValueAtTime, we're inside a condition that guarantees currentTime is 0:
this.gainNode.gain.setValueAtTime(1, this.audioCtx?.currentTime ?? 0)

I tried both, and it was hard to tell the difference, but I think the fade was less severe on the currentTime version. I'm still also confused about the currentTime not moving when we skip issue. Is that still happening?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think your change is correct

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still confused about it too. It does still happen, but I don't think it's actively breaking anything. Worth a follow up investigation at some point, probably, but I think ok to leave for now


this._initContext()
this.audio.preload = 'none'
this.audio.crossOrigin = 'anonymous'
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this fix the tracks that weren't playing?

Copy link
Member Author

Choose a reason for hiding this comment

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

yea

@raymondjacobson raymondjacobson merged commit 493f0d1 into main Nov 16, 2022
@raymondjacobson raymondjacobson deleted the rj-c-1482 branch November 16, 2022 03:22
@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/rj-c-1482

raymondjacobson added a commit that referenced this pull request Nov 16, 2022
* [C-1482] Fix visualizer on mp3 stream

* Clean up

* Fix subtle race condition on visualizer

* Address comments
audius-infra pushed a commit that referenced this pull request Nov 19, 2022
[33185d6] Fix locks again (#2288) Dylan Jeffers
[05b21ce] [C-1505 C-1507 C-1510 C-1511 C-1512] Address Design-QA round 2 (#2285) Dylan Jeffers
[19e4751] [PAY-731] Update streaming logic for premium content (#2282) Saliou Diallo
[29d1c42] Hide tip tile when no tips (#2284) Sebastian Klingler
[8507212] [C-1399] Fix chromecast (#2283) Sebastian Klingler
[2853ca3] [C-1239] Fix bottom-tab-bar highlighting (#2281) Dylan Jeffers
[8d72c34] [PAY-724] Special Error Treatment and libs/sdk bump to 1.0.23 (#2279) Marcus Pasell
[c9eb18d] [C-1515] Allow repeat pushes via useNavigation inside nested navigator (#2280) Sebastian Klingler
[86d42af] Wait for backend account for feature (#2269) Marcus Pasell
[d3b7c45] [C-1496 C-1497 C-1498,C-1499 C-1501 C-1502 C-1503] Address Design-QA (#2278) Dylan Jeffers
[420a7cc] Actually pause audio when AudioPlayer pauses (#2277) Sebastian Klingler
[cfc4379] Fix various lock files (#2276) Dylan Jeffers
[b6db356] [C-1495] Bump target api to 31 (#2275) Sebastian Klingler
[d87f2a0] [C-1419][C-1073] Offline download and playback for collections (#2253) Andrew Mendelsohn
[5d5b035] [C-1494] Listens not being recorded properly? (#2274) Sebastian Klingler
[3e01fba] [C-1465] Address mobile upload tech debt (#2270) Dylan Jeffers
[2721946] Fix makeStyles types (#2268) Andrew Mendelsohn
[8dd185b] [C-1493] Add script to update @audius/sdk (#2271) Dylan Jeffers
[2bab668] Read subscriptions from discovery behind feature flag (#2258) Michelle Brier
[b2ff68a] Fix mobile seek (#2273) Sebastian Klingler
[40e0c14] [Flare-120] Avoid hitting notification settings on each session (#2262) Raymond Jacobson
[cc9a880] Bump stems limit from 5 to 10 (#2272) Raymond Jacobson
[4c82079] [C-981] Add suggested follows to feed if empty (#2231) Raymond Jacobson
[493f0d1] [C-1482] Fix visualizer on mp3 stream (#2259) Raymond Jacobson
[336daf9] [C-1486] Remove mobile-nav-overhaul feature flag (#2264) Dylan Jeffers
[fe90c08] [PAY-695] Get signatures for nft-gated premium tracks (#2245) Saliou Diallo
[125f905] Fix FFMPEG error (Android build) (#2265) nicoback2
[163a584] hotfix: takeLatest was registering after action already fired (#2266) Marcus Pasell
[64bbe4b] [C-1467] Make previous button restart track within 3 seconds (#2263) Sebastian Klingler
[c954633] Fix android builds (#2256) Sebastian Klingler
[47ecbf8] Add audio preview to share to IG video [C-1464] (#2261) nicoback2
[ba29515] [C-1440 C-1457 C-1458 C-1459] Address mobile upload QA (#2254) Dylan Jeffers
[bda3cc6] Create gradient video with ffmpeg [C-1446] [C-1413] (#2250) nicoback2
[cd71ebf] Ensure toasts show below dynamic island (#2257) Sebastian Klingler
[6fbf0ad] Update push notificaiton navigation to work when the app is closed (#2255) Kyle Shanks
[36f7bc4] [PAY-720] [PAY-721] Stripe OnRamp hotfixes (#2218) Marcus Pasell
[3e6e801] Fix playTrack test (#2252) Sebastian Klingler
@AudiusProject AudiusProject deleted a comment from linear bot Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants