Skip to content

fix(ffmpeg): remove redundant log statement #840

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

Merged
merged 1 commit into from
Mar 11, 2025

Conversation

Computerdores
Copy link
Collaborator

Summary

A string for a log message was missing the f to make it a format string.

Tasks Completed

  • Platforms Tested:
    • Windows x86
    • Windows ARM
    • macOS x86
    • macOS ARM
    • Linux x86
    • Linux ARM
  • Tested For:
    • Basic functionality
    • PyInstaller executable

@Computerdores Computerdores moved this to 🍃 Pending Merge in TagStudio Development Mar 7, 2025
@Computerdores Computerdores added Type: Bug Something isn't working as intended Priority: Low Doesn't require immediate attention labels Mar 7, 2025
@CyanVoxel CyanVoxel added the Status: Mergeable The code is ready to be merged label Mar 7, 2025
@CyanVoxel
Copy link
Member

I was curious why this wasn't caught before because I swear that I see valid FFmpeg location logs each time on startup, and then I noticed why - this log statement is redundant and doesn't even need to be here in the first place.

Two log statements in ffmpeg.py already take care of this job, so the log message here can even just be straight up removed 👀

2025-03-07 11:43:18 [info     ] [FFMPEG] Using FFprobe location: ffprobe
2025-03-07 11:43:18 [info     ] [FFMPEG] Using FFmpeg location: ffmpeg

@Computerdores
Copy link
Collaborator Author

Computerdores commented Mar 7, 2025

fair point, it's gone now

@CyanVoxel CyanVoxel added the Status: Blocked This issue or pull request is awaiting the outcome of another issue or pull request label Mar 7, 2025
@CyanVoxel CyanVoxel added this to the Alpha v9.5.2 milestone Mar 7, 2025
@CyanVoxel CyanVoxel removed the Status: Blocked This issue or pull request is awaiting the outcome of another issue or pull request label Mar 10, 2025
@CyanVoxel
Copy link
Member

I apologize for the inconvenience, #844 has now been pulled and a rebase targeting main will be required here now.
Alternatively, I'm looking at fixing some larger issues with this file as it causes a regression with several CMD popups in Windows builds since it circumvents our vendored ffmpeg code. If you wish to rebase this then I'll pull it, otherwise I can take care of this in a future PR - whichever you prefer.

Thank you for your work on this, and sorry again for the disruption!

@Computerdores
Copy link
Collaborator Author

rebase is done

@xarvex xarvex changed the title fix: string wasn't a format string fix(ffmpeg): remove redundant log statement Mar 10, 2025
@xarvex
Copy link
Member

xarvex commented Mar 10, 2025

Actually, are we sure the log is redundant? In testing an environment without FFmpeg, I still get these two log statements, I do not think they actually say FFmpeg is present, but rather that it is being tested:

2025-03-10 10:45:17 [info     ] [FFMPEG] Using FFprobe location: ffprobe
2025-03-10 10:45:17 [info     ] [FFMPEG] Using FFmpeg location: ffmpeg

Later on, I suppose after the check is done, there is this statement (after fixing the format string):

2025-03-10 10:45:20 [info     ] FFmpeg found: False, FFprobe found: False

@CyanVoxel
Copy link
Member

Actually, are we sure the log is redundant? In testing an environment without FFmpeg, I still get these two log statements, I do not think they actually say FFmpeg is present, but rather that it is being tested:

You're right that the original log statement doesn't indicate whether or not it was found or not. However, the underlying code is capable of it and the ffmpeg_checker should be using that backend code (or expanding upon it) instead of being a combination of frontend and backend code that also circumvents fixes done in #436. Overall this is unfortunately just a "tip of the iceberg" issue with some more explanation now in #854

@CyanVoxel CyanVoxel changed the base branch from main to fix-854 March 11, 2025 02:16
Copy link
Member

@CyanVoxel CyanVoxel left a comment

Choose a reason for hiding this comment

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

I'll be merging this into a new branch to use as the basis for a greater set of fixes and refactors involving this corner of the codebase. Thank you for the initial find and work on this!

@CyanVoxel CyanVoxel merged commit 08ec977 into TagStudioDev:fix-854 Mar 11, 2025
5 checks passed
@github-project-automation github-project-automation bot moved this from 🍃 Pending Merge to ✅ Done in TagStudio Development Mar 11, 2025
CyanVoxel added a commit that referenced this pull request Mar 20, 2025
* fix: remove log statement as it is redundant (#840)

* refactor: rework ffmpeg_checker.py

Move backend logic from ffmpeg_checker.py to vendored/ffmpeg.py, add translation strings for ffmpeg_checker, update vendored/ffmpeg.py

* fix: stop ffmpeg cmd windows, fix version outputs

* chore: ensure stdout is cast to str

---------

Co-authored-by: Jann Stute <46534683+Computerdores@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low Doesn't require immediate attention Status: Mergeable The code is ready to be merged Type: Bug Something isn't working as intended
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants