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

fixes for tests #2195

Closed
wants to merge 2 commits into from
Closed

Conversation

antonpetrov145
Copy link

  • set requred version of pillow to 9.5.0
  • test-ffmpeg-reader - change duration number to match video
  • ffmpeg-reader - add displaymatrix to be read as video rotation
  • config set binary to autodetect
  • I have provided code that clearly demonstrates the bug and that only works correctly when applying this fix
  • I have added suitable tests demonstrating a fixed bug or new/changed feature to the test suite in tests/
    addresses issue Test suite fails for most Windows, Linux tests #2183

- set requred version of pillow to 9.5.0
- test-ffmpeg-reader - change duration number to match video
- ffmpeg-reader - add displaymatrix to be read as video rotation
- config set binary to autodetect
@keikoro
Copy link
Collaborator

keikoro commented Jun 7, 2024

@antonpetrov145 Thanks for the contribution!

I'm not sure if removing ffmpeg-imageio for the sake of fixing the tests is the right solution, though. See this comment by Zulko in one of the big threads/discussions about the future of the project (the paragraph before the bullet point summary specifically, which is about dependency management).

@keikoro
Copy link
Collaborator

keikoro commented Jun 7, 2024

Idk, maybe worth throwing your suggested fix into the discussion?

@antonpetrov145
Copy link
Author

antonpetrov145 commented Jun 8, 2024

Thank you for your time to look at the changes I made.

The idea behind setting the ffmpeg binary to auto-detect by default and removing the test is because of an issue that was because of ffmpeg-imageio setting the default binary to exe even on Linux (my other PR) causing Path error on importing moviepy. #2189 #2190

Idk, maybe worth throwing your suggested fix into the discussion?

Yes sure, is it ok for me to write there?

@keikoro
Copy link
Collaborator

keikoro commented Jun 10, 2024

@antonpetrov145 I've now had (more of) a look at the code (and related issues), though haven't been able to test it – am I understanding correctly that changing FFMPEG_BINARY is not actually needed to fix the issue at hand? If so, I'd ask you leave off that particular change and only (force) push the relevant bits of code.

@antonpetrov145
Copy link
Author

antonpetrov145 commented Jun 11, 2024

@keikoro made the change you proposed
Irrelevant: this is irrelevant actually it is - without setting the binary to auto-detect (in code or in ENV) tests are not passing, at least on my side. If you think I'm doing something wrong I'm happy to make the change you propose

@OsaAjani
Copy link
Collaborator

OsaAjani commented Dec 5, 2024

Thank you for your contributions and for reporting issues in this repository. With the release of v2, which introduces significant changes to the codebase and API, we’ve reviewed the backlog of open PRs and issues. Due to the length of the backlog and the likelihood that many of these are either fixed or no longer applicable, we’ve made the decision to close all previous PRs and issues.

If you believe that any of these are still relevant to the current version or if you'd like to reopen a related discussion, please feel free to create a new issue or pull request, referencing the old one.

Thank you for your understanding and continued support!

@OsaAjani OsaAjani closed this Dec 5, 2024
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.

3 participants