search

livepeer / task-runner

Background service that executes tasks from the Livepeer API. Mainly used for VOD.
MIT License
3 stars 2 forks source link

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

Closed gioelecerati closed 2 years ago

gioelecerati commented 2 years ago

Fixes #24

figintern commented 2 years ago

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 commented 2 years ago

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!

victorges commented 2 years ago

@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 commented 2 years ago

It worksssss

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

victorges commented 2 years ago

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?

https://github.com/livepeer/lpms/pull/326

gioelecerati commented 2 years ago

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 commented 2 years ago

Closing as https://github.com/livepeer/lpms/pull/326

victorges commented 2 years ago

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.