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

Just media conv #15

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

EtomicBomb
Copy link
Contributor

No description provided.

@EtomicBomb
Copy link
Contributor Author

I am still working on this one so we shouldn't merge it until its in better shape

@EtomicBomb EtomicBomb force-pushed the just-media-conv branch 4 times, most recently from a01d7fc to 9ba0b95 Compare October 20, 2024 20:33
@vagos vagos force-pushed the main branch 3 times, most recently from 3df98fe to 34faf29 Compare October 20, 2024 23:24
@EtomicBomb EtomicBomb force-pushed the just-media-conv branch 6 times, most recently from 836e8c0 to 649a2ef Compare October 22, 2024 00:47
@vagos
Copy link
Collaborator

vagos commented Oct 22, 2024

@EtomicBomb Good job! I tried running on my end in the container and it both checks fail. Does it pass on your end?

@EtomicBomb
Copy link
Contributor Author

Yeah they both pass. That is curious

@EtomicBomb
Copy link
Contributor Author

EtomicBomb commented Oct 22, 2024

Can you send the md5sum of the inputs and outputs? @vagos

@EtomicBomb
Copy link
Contributor Author

EtomicBomb commented Oct 22, 2024

I pulled ghcr.io/binpash/benchmarks:main:
docker build -t bench2 -f- . <<EOF
FROM ghcr.io/binpash/benchmarks:main
COPY . .
RUN ["bash"]
EOF

and ran ./main.sh media-conv --small, ./main.sh media-conv, and everything passes. Can you provide more info about the error you're experiencing and about your workflow @vagos?

@vagos
Copy link
Collaborator

vagos commented Oct 22, 2024

I see this one is also failing in CI

@EtomicBomb
Copy link
Contributor Author

True. Testing locally on an ubuntu 22.04 image I get all of the mp3s failing to match.

@EtomicBomb
Copy link
Contributor Author

EtomicBomb commented Oct 22, 2024

I looked at one of the mp3s. Running md5sum on the whole file, the hashes don't match between debian and ubuntu. Hashing just the audio streams with ffmpeg -i ./results/to_mp3.small/file_example_WAV_10MG.wav.mp3 -map 0:a -f md5 - 2>/dev/null makes them match. The images have different versions of ffmpeg. I wonder if hashing the audio streams is robust enough.

ffmpeg version 4.4.2-0ubuntu0.22.04.1 Copyright (c) 2000-2021 the FFmpeg developers
built with gcc 11 (Ubuntu 11.2.0-19ubuntu1)

(from ubuntu-latest)

vs.

ffmpeg version 5.1.6-0+deb12u1 Copyright (c) 2000-2024 the FFmpeg developers
built with gcc 12 (Debian 12.2.0-14)

(from debian 12.7)

@vagos
Copy link
Collaborator

vagos commented Oct 22, 2024

Great catch! Do you think you can make the CI use our container?

Hashing the audio streams is 100% okay (I think this will probably save us headaches in the long run).
I imagine this just requires changing the verify.sh script.

@vagos
Copy link
Collaborator

vagos commented Oct 22, 2024

If we also want to use our container for the CI, make a separate PR :)

@EtomicBomb
Copy link
Contributor Author

@vagos The benchmark passes CI now.

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.

2 participants