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

Audio analysis: mediorum changes #8536

Merged
merged 15 commits into from
May 30, 2024

Conversation

michellebrier
Copy link
Contributor

@michellebrier michellebrier commented May 17, 2024

Description

  • Add cols to uploads. Analysis results are in 1 column so it should be extensible
  • Allow 1 minute to analyze audio in mediorum after transcoding completes
  • Analysis failures/timeouts/errors do not block the upload. It gets marked as done regardless of analysis result
  • Add endpoint to retry failed or missing audio analyses
  • Installing Essentia in the mediorum Dockerfile was a huge pain and the Essentia docker images are very outdated and different than the Essentia Python I used in my test script so I abandoned this library in favor of https://github.com/mixxxdj/libkeyfinder/tree/main (per Marcus' recommendation) for key detection and https://aubio.org/ for BPM detection. We can always replace these tools later if we find something better.

How Has This Been Tested?

  • Run locally via audius-compose up mediorum
  • Verify analysis runs
image - Verify server changes - Make code change to sleep for 2 mins when handling audio analysis job, verify 1 min timeout applies and upload completes after 1 min - Verify prod dockerfile builds

Copy link

changeset-bot bot commented May 17, 2024

⚠️ No Changeset found

Latest commit: 35fea74

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@michellebrier michellebrier marked this pull request as ready for review May 24, 2024 11:14
@michellebrier michellebrier requested a review from stereosteve May 24, 2024 11:14
@michellebrier michellebrier requested a review from sliptype May 24, 2024 16:37
RUN go build
RUN go build -o mediorum-cmd cmd/main.go

FROM alpine:3.17 AS final

RUN apk add curl ffmpeg
RUN apk add curl ffmpeg libsndfile python3-dev py3-pip build-base
Copy link
Contributor

Choose a reason for hiding this comment

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

cool to see basically all the same deps as https://github.com/swesterfeld/audiowmark which @stereosteve and I have been messing with

Copy link
Contributor

@sliptype sliptype left a comment

Choose a reason for hiding this comment

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

SO sick! Gonna have to defer to @stereosteve for the go stuff because I don't have much/any experience with it. But generally this looks good to me

mediorum/server/audio_analysis.go Outdated Show resolved Hide resolved
}

func (ss *MediorumServer) analyzeKey(filename string) (string, error) {
cmd := exec.Command("/bin/analyze-key", filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

neat

mediorum/server/db.go Show resolved Hide resolved
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.

not confident enough to approve without someone like steve reviewing, but this is looking dope

mediorum/cpp/keyfinder.cpp Outdated Show resolved Hide resolved
mediorum/server/audio_analysis.go Show resolved Hide resolved
}
}

func (ss *MediorumServer) findMissedAnalysisJobs(work chan *Upload, myHost string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

will leave this function for steve :)


// poll periodically for uploads that slipped thru the cracks.
// mark uploads with timed out analyses as done
for {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing a lot of context, what is the main thing this solves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this handles cases where the 1st mirror misses the job for whatever reason (most likely, the node went down). in such instances the next mirrors pick up the job. after 1 min, if the job is still not finished, it times out and gets marked as done so the client can proceed with the upload

the transcoder has similar logic

func (ss *MediorumServer) findMissedJobs(work chan *Upload, myHost string, retranscode bool) {

@stereosteve
Copy link
Contributor

stereosteve commented May 28, 2024

This looks pretty great!

My one thought is... we're adding a new background polling thing and a audio_analysis state before done... but if the client is going to poll the upload status and look for done... we might as well do the audio analysis step right after transcode, right?

I'm not super familiar with the client upload flow, but if it is polling for done the client will block during the audio_analysis step anyway.

I think this is okay if it's decently fast. Since it uses the crudr hook I think it should actually kick the job off right away without waiting for polling interval.

Have you tested end to end with client? How long does the analysis take on a 3 minute song and a 1 hour song?


I see here it Allow 1 minute to analyze audio in mediorum after transcoding completes.

Anyway one option might be... instead of introducing a new processing state, we query for template='audio' and status = 'done' and AudioAnalysisStatus is null type of thing.

@michellebrier
Copy link
Contributor Author

michellebrier commented May 29, 2024

This looks pretty great!

My one thought is... we're adding a new background polling thing and a audio_analysis state before done... but if the client is going to poll the upload status and look for done... we might as well do the audio analysis step right after transcode, right?

I'm not super familiar with the client upload flow, but if it is polling for done the client will block during the audio_analysis step anyway.

I think this is okay if it's decently fast. Since it uses the crudr hook I think it should actually kick the job off right away without waiting for polling interval.

Have you tested end to end with client? How long does the analysis take on a 3 minute song and a 1 hour song?

I see here it Allow 1 minute to analyze audio in mediorum after transcoding completes.

so the idea is, when a node gets a new upload, it crudr broadcasts a new upload with status=new. the first mirror (or second or third) starts the transcoding job as usual and sets the usual transcoding statuses. then, if this is a new audio upload, once transcoding completes, the transcoding node sets status=audio_analysis (see the change here). this triggers the first mirror (or second or third) to pick up the audio analysis job similarly to how transcoding works. it sets status=busy_audio_analysis while working then sets status=done after the job finishes, the job errors, or 1 minute passes, whichever comes first. the client continues to poll for done as before. so it runs sequentially wrt the transcoding and the client is blocked by the audio analysis step for up to an additional minute. audio analysis errors do not make the upload fail, they just get recorded in audio_analysis_error on the upload.

have not tested e2e yet, only locally via a POST to the mediorum node, but analysis typically takes a few seconds on songs that are a few minutes long. i wanted to use the essentia python library which was very performant (1-2s) for even v long songs, but it was not playing nice with docker, so im using libkeyfinder for the key and aubio for bpm. aubio takes about 1-2 seconds even on long tracks (i tested a 45min track) but libkeyfinder was slower on a long track. still < 1 minute. in any case it wont block the upload for more than 1 minute, and the plan is for discovery to run a job that retries failed analyses regularly with a longer timeout interval.

are those numbers decently fast in your opinion?

my plan for this PR is, as long as everyone is ok with adding up to a minute to the upload time to allow for audio analysis, to merge without any discovery or client changes so that mediorum analyzes but nothing gets saved in discovery yet. that way i can see what the analyses look like with real data with these key/bpm libraries. if its way too slow or inaccurate i can swap out for some different libraries (but keep this mediorum processing state flow).

Anyway one option might be... instead of introducing a new processing state, we query for template='audio' and status = 'done' and AudioAnalysisStatus is null type of thing.

do you mean discovery runs something like this query and kicks off audio analysis this way? or mediorum queries this regularly, and discovery periodically checks for newly completed audio analyses? i decided to add it as a blocking part of the upload flow so that the client can poll all the audio artifacts as usual (and the key/bpm data shows up immediately after the upload succeeds) and to avoid adding a discovery / content node dependency.

@michellebrier michellebrier merged commit 2ceae28 into main May 30, 2024
10 of 11 checks passed
@michellebrier michellebrier deleted the mbrier/proto-1827/mediorum-audio-analysis branch May 30, 2024 21:29
@michellebrier michellebrier restored the mbrier/proto-1827/mediorum-audio-analysis branch May 31, 2024 00:01
audius-infra pushed a commit that referenced this pull request May 31, 2024
[7343561] [PAY-2842][PAY-2780][PAY-3069] Fix authorized endpoints not supporting manager mode (#8646) Randy Schott
[ddaa1b3] ⚠️ Third Party Wallet Support [PAY-2949][PAY-2948][PAY-2950] (#8611) Marcus Pasell
[c15f20d] Fix prod mediorum dockerfile to have keyfinder dependency (#8666) Michelle Brier
[7fe9451] Fix solana-relay logging, fix eslint config (#8663) Marcus Pasell
[a495f42] [C-4453] Fix notification email image and layout (#8647) Isaac Solo
[2ceae28] Audio analysis: mediorum changes (#8536) Michelle Brier
[15a34b3] [C-4353] Add recent searches to search v2 (#8615) Sebastian Klingler
[0050b29] Fix sdk typecheck (#8656) Sebastian Klingler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants