Clip Refactor (move Timeline Drawing code to Clip) #571
Conversation
- Making Clip a proper Reader (so it can be used directly, instead of a Timeline)
…()->MAX_HEIGHT out of the Cilp class. GetFrame() now has an overload which specifies the width, height, and samples needed. Otherwise, it returns the Clip image based on the source reader (width, height, num samples).
…need to replace the Crop functionality with a more robust cropping tool. Also, updated Timeline to use the MaxWidth/MaxHeight settings when calling the clip (since those are set when the screen is resized).
jonoomph
changed the title
Codecov Report
@@ Coverage Diff @@
## develop #571 +/- ##
===========================================
+ Coverage 49.45% 51.05% +1.59%
===========================================
Files 129 130 +1
Lines 10261 10443 +182
===========================================
+ Hits 5075 5332 +257
+ Misses 5186 5111 -75
Continue to review full report at Codecov.
|
…w a Clip access to the parent timeline instance (if available), and thus, certain properties (preview size, timeline FPS, etc...). This allows for a simpler rendering of Clip keyframes (during the Clip::GetFrame method), and a simpler Timeline class, that can change the preview window size dynamically and no longer requires a Singleton Settings class. - Also removed "crop" from Clip class, as it was never implmeneted correctly, and we have a fully functional "crop" effect when needed - Added caching to Clip class, to optimize previewing of cached frames (much faster than previous)
# Conflicts: # include/Clip.h # include/ReaderBase.h # include/Timeline.h # src/Clip.cpp # src/FFmpegReader.cpp # src/QtImageReader.cpp # src/ReaderBase.cpp
…hen exporting to different fps The FrameMapper class now receives the updated clip position and returns the correct amount of samples for a given frame number
Implemented position remapper inside FrameMapper to fix audio noise when exporting to different fps
…start and position (if any)
…ich still seems to benefit from performance, but keeps the byte order the same as before. win win
…ll colors (since we have switched to a premulitplied alpha format)
# Conflicts: # src/CacheDisk.cpp # src/Clip.cpp # src/Frame.cpp # src/QtHtmlReader.cpp # src/QtImageReader.cpp # src/QtTextReader.cpp # src/effects/Bars.cpp # src/effects/Crop.cpp
- Making Clip a proper Reader (so it can be used directly, instead of a Timeline)
…()->MAX_HEIGHT out of the Cilp class. GetFrame() now has an overload which specifies the width, height, and samples needed. Otherwise, it returns the Clip image based on the source reader (width, height, num samples).
…need to replace the Crop functionality with a more robust cropping tool. Also, updated Timeline to use the MaxWidth/MaxHeight settings when calling the clip (since those are set when the screen is resized).
…w a Clip access to the parent timeline instance (if available), and thus, certain properties (preview size, timeline FPS, etc...). This allows for a simpler rendering of Clip keyframes (during the Clip::GetFrame method), and a simpler Timeline class, that can change the preview window size dynamically and no longer requires a Singleton Settings class. - Also removed "crop" from Clip class, as it was never implmeneted correctly, and we have a fully functional "crop" effect when needed - Added caching to Clip class, to optimize previewing of cached frames (much faster than previous)
…hen exporting to different fps The FrameMapper class now receives the updated clip position and returns the correct amount of samples for a given frame number
…start and position (if any)
…ich still seems to benefit from performance, but keeps the byte order the same as before. win win
|
Still cleaning up the conflict-resolution here, I should have this back to passing within the hour. |
|
Hrm. This could become an issue, if anyone ever decides to actually use the Ruby bindings...
|
I did notice this. Hmmm, I wonder how this will actually manifest at runtime. It apparently still compiles the swig module for Ruby, but somehow with missing multiple inheritance? Sounds like it fail to build the Ruby binding I'm missing something. |
| // Calculate ratio of source size to project size | ||
| // Even with no scaling, previews need to be adjusted correctly | ||
| // (otherwise NONE scaling draws the frame image outside of the preview) | ||
| float source_width_ratio = source_size.width() / float(width); | ||
| float source_height_ratio = source_size.height() / float(height); | ||
| source_size.scale(width * source_width_ratio, height * source_height_ratio, Qt::KeepAspectRatio); |
There was a problem hiding this comment.
This is exactly equivalent to:
source_size.scale(source_size.width(), source_size.height(), Qt::KeepAspectRatio);The two widths and heights in the ratios and in the arguments cancel each other out. So, it's a complete no-op.
There was a problem hiding this comment.
(By canceling, I mean:)
width * source_width_ratio
= widthsource_size.width() / float(width)
= source_size.width()
Oh, it builds just fine, but what comes out... ain't right. For instance, the ruby $ cd build/bindings/ruby
$ irb -I . -ropenshot
irb(main):002:0> c = Openshot::Clip.new('test.mp4')
irb(main):003:0> c.Open()
=> nil
irb(main):004:0> c.IsOpen()
=> true
irb(main):005:0> c.Position()
=> 0.0
irb(main):006:0> c.Duration()
=> 3.3366665840148926
irb(main):007:0> c.DisplayInfo()
Traceback (most recent call last):
4: from /usr/bin/irb:23:in `<main>'
3: from /usr/bin/irb:23:in `load'
2: from /home/ferd/.gem/ruby/gems/irb-1.2.4/exe/irb:11:in `<top (required)>'
1: from (irb):7
NoMethodError (undefined method `DisplayInfo' for #<Openshot::Clip:0x00005618a4aafee8>)
Did you mean? display
irb(main):008:0> c.info
Traceback (most recent call last):
5: from /usr/bin/irb:23:in `<main>'
4: from /usr/bin/irb:23:in `load'
3: from /home/ferd/.gem/ruby/gems/irb-1.2.4/exe/irb:11:in `<top (required)>'
2: from (irb):7
1: from (irb):8:in `rescue in irb_binding'
NoMethodError (undefined method `info' for #<Openshot::Clip:0x00005618a4aafee8>)
irb(main):009:0> |
Adding suggestions from @ferdnyc Co-authored-by: Frank Dana <ferdnyc@gmail.com>
Applying some awesome improvements from @ferdnyc, thx! Co-authored-by: Frank Dana <ferdnyc@gmail.com>
- Removing transformed == true boolean (Qt should be smart enough to optimize for blank transforms) - Fixing regression from TimelineBase import
|
@ferdnyc Regarding Ruby, I don't have an easy solution for this one. I tried a ton of different approaches to access the parent timeline of a clip. But since the Timeline class depends on the Clip, the Clip interface can't also depend on the Timeline. Creating a base class of the Timeline to expose a few of the parameters needed seemed to make the most sense. However, simple forward declarations don't work, since we are also access some members of the class. I had to revert the one suggestion because it fails to compile. In summary, I've tried a bunch of things, and what we have now seems the simplest (and only) solution I could come up with, but I'm open to ideas. |
Hm, really? In the header? I missed that, somehow. (In the implementation, sure. Part of what I was suggesting was that we add Well, anyway, that's largely housekeeping. But on this...
Yeah, I've got nothin' either, when it comes to the multiple inheritance problem. (Which the forward declaration suggestion wouldn't solve anyway, that was just about the source-file dependency tree.) I did have one awful thought last night, but I hated myself immediately for it so I didn't bring it up: ...What if both TimelineBase and ClipBase were derived from ReaderBase? It could possibly solve the Ruby problem (at the expense of, you know, sanity), as it would eliminate the multiple inheritance. (It seems like it would, anyway. Not 100% sure. Like I said, I felt dirty as soon as I thought it, so I kind of pushed it way down and tried not to think about it.) |
That is not a terrible idea. In effect, both Clip and Timeline are readers now. We would just need to move some things around. But I'll keep that a separate idea from this PR. But I do agree it would solve the problem! Nice |
|
Heh. libopenshot 0.3.0, the "It's Readers all the way down" edition. 😆 |
…ests (which throw warnings)
…no gaps/pops, and offset FrameMapper clips don't use the wrong # of audio samples
…m, and FFmpegReaders with a Clip/Timeline associated with them.
…f cache vs display frame #. Also found a mutex that was needed, to prevent crashing when the video thread calls timeline::GetFrame at certain times... colliding with another thread (and independent of OpenMP).
|
Finally merging this branch, as I have other branches which depend on this one. If this causes any huge issues, we can open up another PR. 🤞 |
|
Fingers crossed emoji is right next to the middle finger one... lol. That is dangerous! |
jonoomph
changed the title
|
@ferdnyc You might be right about the reverse playback. I will look into that. In theory, the direction of the player should drive the direction of the clip caching. Yes, it is a bit scorched earth (and was somewhat experimental), but it also solves some problems (as scorching the earth tends to do). We can always revert it, if the problems outweigh the fixes. This was really designed to solve 1 thing:
But in my testing (including the changes to video caching), I found things much more responsive and continuous in OpenShot. So I thought, dang, maybe this is better. But maybe it's not. 😬 |
Yeah, but sorta-not-really. You can't escape the basic reality that encoded video is tuned to be read primarily in one direction — except for a few specialty transport formats or full-frame raw video streams (@ gigabytes-per-second of bandwidth), video wants to be read "in, like... forwards". I think what the reverse player would have to do, to cache most effectively, is figure out the "frame chunk size" that's going to be produced by the reader (based on the processor count, etc.) and then keep jumping that many frames "ahead of" the "end" of the cache (really, jumping before the beginning of the cache), stuffing N new frames onto the front in one go, and then jumping again. Like a video-frame-hoarding Scott Bakula.
I wouldn't be surprised if it is, for the most degenerate case where OpenShot is easily able to keep up with the input. Those are the times when you really don't need a cache, and a small readahead buffer is plenty. But as soon as the readahead starts falling behind and can't keep up with the playhead, then everything goes to hell fast. That's true even with the cache, to some extent (unfortunately), but at least with a cache you can give it a run to fill the buffer, then back up and play from the cache at reasonable speed. |
...And even doing that, there are going to be a ton of situations where it takes the next leap and ends up smack in the middle of |
This will likely be absorbed into another PR before I'm done. But I wanted to add some visibility to the work.