Skip to content

Fix typo in box merge#4

Merged
ladaapp merged 3 commits into
ladaapp:mainfrom
ratbunch:box_merge_fix
Dec 2, 2024
Merged

Fix typo in box merge#4
ladaapp merged 3 commits into
ladaapp:mainfrom
ratbunch:box_merge_fix

Conversation

@ratbunch
Copy link
Copy Markdown
Contributor

@ratbunch ratbunch commented Dec 1, 2024

Fixed box merge logic to correctly merge the boxes into a larger box. Looks like a typo. Was causing mosaics to flash for a frame or causing some mosaics to not be cleaned when boxes were nearby one another.

Fixed box merge logic to correctly merge the boxes into a larger box. Looks like a typo. Was causing mosaics to flash for a frame or causing some mosaics to not be cleaned when boxes were nearby one another.
@ratbunch
Copy link
Copy Markdown
Contributor Author

ratbunch commented Dec 1, 2024

There is still another issue I'm seeing sometimes when the merged boxes/masks go from being combined to being split. One of them doesn't get cleaned for a single frame sometimes. Still trying to find the source of that though.

Edit: This turned out to just be the model not detecting a mosaic in that frame. Just happened to be at a frame where a merge was happening previously.

… certain videos framerate being off and exceptions being thrown. Should attempt to process the video if this occurs. Seems to be happening because of a 'off by one' frame number on some videos. Harmless to catch exception and continue
@ratbunch
Copy link
Copy Markdown
Contributor Author

ratbunch commented Dec 1, 2024

Added a try/except to the main cli portion. There seems to be an 'off by 1' issue with frame numbers in the code that is completely killing the progress at the end of a video. Maybe we should catch everything here. No reason to lose the progress of an entire run because of something. Should always attempt to produce a result. It's likely happening with variable framerate videos and the next() calls throwing exceptions when the frame count is off.

@ladaapp
Copy link
Copy Markdown
Owner

ladaapp commented Dec 1, 2024

Regarding the frame count issue:

We get the frame count in video_utils::get_video_meta_data via ffprobe ``. It tries to read it from the files metadata but if its not available then we get it via opencv which itself estimates it by frame_duration * fps which could indeed be not totally correct. For the files I tested it with this was fine but if you've hit files where this is an issue it would be great to have something more robust.

I see two options, either we 1) count the frames (in ffprobe this could be done with the -count_frames argument) or we 2) catch the error and handle it gracefully.

  1. Would be easiest to implement: replace the opencv (time-based calculation of of frame count) fallback with the ffprobe using the count_frames arg. This will get us an accurate frame count but will be super slow for long videos as it will need to decode the whole video in order to count all frames one-by-one

  2. catch the exception where we call next(video_utils::VideoReader.frames()). if this happened the frame count we got earlier was incorrect and we already processed all frames of the input video. Now we need to properly clean up (same logic as we would have hit the last frame according to our frame count) -> e.g. complete the currently running scene.

Let's go with 2) as 1) would just be too slow for files longer than a few minutes.

@ratbunch
Copy link
Copy Markdown
Contributor Author

ratbunch commented Dec 1, 2024

Let's go with 2) as 1) would just be too slow for files longer than a few minutes.

Agreed. It's easier to just catch the case when we are off by one. Where I added is probably not the place you want to do it though. Feel free to fix that outside this PR because I'm still getting familiar with the code.

Biggest issue is the min/max typo though. If you want to cherry-pick that from here or add it yourself. Fix greatly reduces "flashing" when boxes are merging/splitting. Though, like I said, there is still "flashing" mosaics showing sometimes for another reason I'm trying to pinpoint. It's happening on box merge frame still but I don't know why.

@ladaapp
Copy link
Copy Markdown
Owner

ladaapp commented Dec 2, 2024

@wheezy1749 There is a non-zero chance I fixed it in 76a8fe5
Can you give it a try if it works for your files?

If can revert your changes in this PR except the fix for the min/box box thingy I'd be happy to merge it.

…ues with certain videos framerate being off and exceptions being thrown. Should attempt to process the video if this occurs. Seems to be happening because of a 'off by one' frame number on some videos. Harmless to catch exception and continue"

This reverts commit b682d88.
@ratbunch
Copy link
Copy Markdown
Contributor Author

ratbunch commented Dec 2, 2024

@wheezy1749 There is a non-zero chance I fixed it in 76a8fe5 Can you give it a try if it works for your files?

Sure. I'll test it soon.
Edit: Seems to have fixed the issue. At least on the video that was crashing the script previously.

If can revert your changes in this PR except the fix for the min/box box thingy I'd be happy to merge it.

Reverted to just the min/max fix.

@ladaapp ladaapp merged commit 7cb8312 into ladaapp:main Dec 2, 2024
@ladaapp
Copy link
Copy Markdown
Owner

ladaapp commented Dec 2, 2024

Done, thanks for the fix!

ConnoisseurProtege pushed a commit to ConnoisseurProtege/lada that referenced this pull request Apr 23, 2025
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.

2 participants