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

[videoFilter] New filter: Reverse #317

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

szlldm
Copy link
Contributor

@szlldm szlldm commented Aug 27, 2021

Play a short section backward.

@eumagga0x2a
Copy link
Collaborator

Thank you very much, a couple of points after a cursory try-out:

  1. The build fails on macOS for the trivial reason of missing ADM_coreVideoFilter.h include in cli/DIA_reverse.cpp (with DIA_coreToolkit.h and math.h includes being unneeded).
  2. I can imagine that Avidemux toolkit is still too limited to implement the configuration dialog in toolkit instead of directly in Qt. But in this case it might be tempting to extend the toolkit, at least middle-term, IMHO.
  3. When previewing the filter in the main window, audio is strongly out of sync (delayed!) after the end of filter scope. Audio being ahead of video is easily possible with slow filters, but getting behind is probably a bug.
  4. When starting preview in the main window from a point within the range, the filter seems to determine the range from relative time rather than absolute. Unsure, though. Something is not right here.
  5. Preview after seek into the range in the filter manager which is acknowledged as not working, would be nice to have.
  6. I wonder whether the cache file location and name could be made user-defined.
  7. QString text = QString(QT_TRANSLATE_NOOP("reverse","Time scope: ")); and similar cases: I guess the constructor in assignment is redundant.
  8. Would it be possible to catch malloc failure during preview rather than assert and crash?

@szlldm
Copy link
Contributor Author

szlldm commented Aug 29, 2021

The build fails on macOS for the trivial reason of missing ADM_coreVideoFilter.h include in cli/DIA_reverse.cpp (with DIA_coreToolkit.h and math.h includes being unneeded).

Should be fixed

When previewing the filter in the main window, audio is strongly out of sync (delayed!) after the end of filter scope. Audio being ahead of video is easily possible with slow filters, but getting behind is probably a bug.

I tried the scenario with more than one video. Here works as expected, after the scope the audio is in sync with the video.

When starting preview in the main window from a point within the range, the filter seems to determine the range from relative time rather than absolute. Unsure, though. Something is not right here.

I think it is, due to not gettting a call to goToTime, which would invalidate the filter. IDK if this could be fixed at all.

Preview after seek into the range in the filter manager which is acknowledged as not working, would be nice to have.

I dont think it would be possible.

I wonder whether the cache file location and name could be made user-defined.

It is a temporary file, imho it is more convenient this way. Also it follows the convention of the .idx files (of .ts files).

Would it be possible to catch malloc failure during preview rather than assert and crash?

I will think about it.

@szlldm
Copy link
Contributor Author

szlldm commented Aug 29, 2021

I think, the preview problems could be addressed, as warn the users, not use previews after instantiating this filter.
Without proper backend support the preview issues couldnt be addressed, but IMHO with the current time based backend it wont be possible (as opposed to pre-2.6 frame based backend.)

Malloc failures could be indicated by GUI_Error_HIG(). Does it works in cli mode? What sould we do, if malloc fails during encoding?

@szlldm
Copy link
Contributor Author

szlldm commented Aug 31, 2021

When starting preview in the main window from a point within the range, the filter seems to determine the range from relative time rather than absolute. Unsure, though. Something is not right here.

I have figured this out: somehow the handed over image PTS start at zero when start palying filtered video in the main window, regardless of what is should be.
You can easily test it, just instantiate a Misc./PrintInfo filter. It also will show the incorrect PTS.

@eumagga0x2a
Copy link
Collaborator

somehow the handed over image PTS start at zero when start palying filtered video in the main window, regardless of what is should be.

I think this is the expected behaviour, pts are relative to the bridge range. There are functions to get the absolute timing.

@szlldm
Copy link
Contributor Author

szlldm commented Aug 31, 2021

Fixed the absolute Pts issue, warning message get refined.
Show green screen if start playing inside the time scope.

I've managed to get the audio desync issue. It happens when the buffering takes more times, and the audio play get suspended during that. If it can buffer (decode) fast enough, audio stay in sync after the filter's scope.

@szlldm
Copy link
Contributor Author

szlldm commented Aug 31, 2021

I have tried GUI_Error_HIG(), but it does not work even in main window's preview.
Do you have a better idea for handling malloc failure?

@szlldm
Copy link
Contributor Author

szlldm commented Sep 1, 2021

So it looks like i have figured it out, how to replace assert with a graceful solution.
If error happens, it returns with a last frame colored red + short error message, also write message to the log, then at the next getNextFrame call returns false and interrupts play or encoding.

@eumagga0x2a
Copy link
Collaborator

The malloc behaviour on macOS is troublesome: it doesn't fail but the system simply swaps to death trying to reverse a 3 minutes long 1080p@50fps video at default settings except of buffering to memory instead of to a file (well, I managed to kill Avidemux as it passed through ~17 GiB of memory on a MBP with 8 GiB RAM).

I think the design is not viable on macOS :-(

@szlldm
Copy link
Contributor Author

szlldm commented Sep 15, 2021

I think the design is not viable on macOS :-(

Did the estimated RAM usage (at the filter dialog) correlated with the above number?
If yes, then it is a user error by ignoring the warnings :)

@eumagga0x2a
Copy link
Collaborator

Did the estimated RAM usage (at the filter dialog) correlated with the above number?

No, it was completely impossibly high for this machine (~28 GiB). The goal was to trigger a malloc failure.

If yes, then it is a user error by ignoring the warnings :)

Yes and no. User errors need to be tested too :) It is fine to risk an app crash / app being killed by an OOM guard or so. But inducing a system hang, potentially destroying important work in other applications is unacceptable, no matter what.

@szlldm
Copy link
Contributor Author

szlldm commented Sep 15, 2021

Memory handling is similar on Linux, it allows allocate more memory than actual RAM, because technially swap is also memory (albeit very slow). Escalate the situation further, the system could allow allocation of even larger chunk, more than RAM+swap, due to overcommitting.

@eumagga0x2a
Copy link
Collaborator

On that macOS system, there was not enough disk space to accomodate the necessary swap, so I hoped the operation to fail outright. There is some macOS-specific stuff like memory compression which may have played a role, however.

Either way, this doesn't make the situation easier.

@szlldm
Copy link
Contributor Author

szlldm commented Sep 15, 2021

IMHO the warning message say all. Yes, this filter can hang your system.
Same as dont put your soaked cat into the microwave to dry.

@eumagga0x2a
Copy link
Collaborator

IMHO the warning message say all. Yes, this filter can hang your system.

If you ever get to see this message, i.e. if you add the filter via configuration dialog rather than via a script.

Same as dont put your soaked cat into the microwave to dry.

I would expect the poor animal to die a horrible death and the microwave possibly to get broken, but not one's doctoral thesis to be destroyed, if you allow such analogy.

@szlldm
Copy link
Contributor Author

szlldm commented Sep 15, 2021

If you ever get to see this message, i.e. if you add the filter via configuration dialog rather than via a script.

There is a high probability that if no more than once the user will see it.
The chance of crashing the system also applies to ffmpeg's reverse filter, it also buffer to memory (but only to memory, no file option).

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