Skip to content

Default colorspace for CPU decoder is now ITU709 #5291

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

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

Conversation

bjuncek
Copy link
Contributor

@bjuncek bjuncek commented Jan 26, 2022

Addressing #5245

@facebook-github-bot
Copy link

facebook-github-bot commented Jan 26, 2022

💊 CI failures summary and remediations

As of commit 0f8a072 (more details on the Dr. CI page):



1 failure not recognized by patterns:

Job Step Action
CircleCI cmake_macos_cpu curl -o conda.sh https://repo.anaconda.com/miniconda/Miniconda3-latest-MacOSX-x86_64.sh
sh conda.sh -b
source $HOME/miniconda3/bin/activate
conda install -yq conda-build cmake
packaging/build_cmake.sh
🔁 rerun

🚧 4 fixed upstream failures:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

Copy link
Contributor

@prabhat00155 prabhat00155 left a comment

Choose a reason for hiding this comment

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

Thanks @bjuncek, this looks good to me. I have a few minor comments for you to consider.

@prabhat00155
Copy link
Contributor

@bjuncek Also, could you fix the linter error please?

@prabhat00155
Copy link
Contributor

Internal tests: D33830672

Copy link
Contributor

@prabhat00155 prabhat00155 left a comment

Choose a reason for hiding this comment

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

Some internal tests are failing, blocking merge for now.

Copy link
Contributor Author

@bjuncek bjuncek left a comment

Choose a reason for hiding this comment

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

I think I've addressed most of the concerns, and have added additional coverage for video_reader tests

@bjuncek
Copy link
Contributor Author

bjuncek commented Apr 7, 2022

@prabhat00155 @datumbox any updates on this?

@datumbox
Copy link
Contributor

datumbox commented Apr 7, 2022

@bjuncek I don't have any context about this PR. Not sure if @prabhat00155 or @fmassa can help here.

BTW there is a conflict in one of the test files, I assume you will take care of it after you get the OK on principle for the direction of the PR?

@prabhat00155
Copy link
Contributor

@datumbox This PR would need to be imported into Phabricator to make sure all the internals tests are fine, before merging here, as we are changing the colour space for CPU decoder, which means the output values would be different. Last time, I saw some internal test failures on importing this(D33830672).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants