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

prepare: effectiveProfile to avoid dimensions shorter than 145 pixels #29

Merged
merged 2 commits into from
Jun 14, 2022

Conversation

gioelecerati
Copy link
Member

Fixes #24

@gioelecerati gioelecerati requested a review from a team as a code owner May 12, 2022 19:03
@figintern
Copy link
Collaborator

figintern commented May 12, 2022

LGTM! Awesome you were able to root cause the 145 px restriction.

Were you able to do some testing ie. with the Jellyfish video? Would be helpful to add to the PR description.

This is not the fix for the E2E testing video correct? Sounds like we are still unsure on root cause for that.

@victorges
Copy link
Member

Were you able to do some testing ie. with the Jellyfish video? Would be helpful to add to the PR description.

AFAIK the transcode-cli worked with a 258p profile and failed with a 257p profile, which is exactly the theorical threshold to get the effective width to be 145px or 144px.

This is not the fix for the E2E testing video correct? Sounds like we are still unsure on root cause for that.

Correct! The fix will likely be on the orchestrators instead, since we were just able to reproduce that the issue only started happening on the version 0.5.30. On go-livepeer@0.5.29 it works, and with this fix we should theoretically have no assets failing transcode!

Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM with some nits and a very small change! (>➡️>=)

task/prepare.go Outdated Show resolved Hide resolved
task/prepare.go Outdated Show resolved Hide resolved
task/prepare.go Show resolved Hide resolved
@victorges victorges force-pushed the gio/prepare/effectiveprofile branch from b246e8e to 6cc050f Compare May 12, 2022 21:58
@victorges
Copy link
Member

@tqian1 Just rebased this on top of main and set the dev branch to point to it!

Meaning that, if we've already found all the issues, there should be no invalid argument happening in staging anymore!

@victorges
Copy link
Member

It worksssss

https://lvpr.tv/?monster&recording=d7b8451f-4ddc-4e07-931a-8f6860ea5569

@victorges
Copy link
Member

Actually seems like the most reliable fix for this will actually be in lpms, and they already have a fix in-flight! Should we close this one?

livepeer/lpms#326

@gioelecerati
Copy link
Member Author

Actually seems like the most reliable fix for this will actually be in lpms, and they already have a fix in-flight! Should we close this one?

livepeer/lpms#326

Yes, if it's fixed there there's no need for this hardcoded fix

@gioelecerati
Copy link
Member Author

Closing as livepeer/lpms#326

@victorges
Copy link
Member

The fix in lpms is not yet merged, and I believe we need this to be able to make the prepare task fatal (#40). As such, I'm reopening, merging, and will be deploying this tomorrow so we don't get so many valid videos that would fail in our processing.

This should be reverted as soon as the lpms fix is merged and deployed on our Os.

@victorges victorges reopened this Jun 14, 2022
@victorges victorges force-pushed the gio/prepare/effectiveprofile branch from 1681af8 to baa0f2e Compare June 14, 2022 22:49
@victorges victorges merged commit 364297c into main Jun 14, 2022
@victorges victorges deleted the gio/prepare/effectiveprofile branch June 14, 2022 22:52
figintern added a commit that referenced this pull request Jun 23, 2022
figintern added a commit that referenced this pull request Jun 23, 2022
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.

Fix "invalid argument" error for some assets transcode
3 participants