Fix none double scaling

[jeffski]

This PR attempts to resolve the following issue: #754

There is a problem where QtImageReader and FFmpegReader scale the image or video. The scaling is then done again in the Clip. When the scale type is set to SCALE_NONE, the image or video appears to be scaled twice when it shouldn't be scaled. The issue description demonstrates this.

Also note that other readers, like QtHtmlReader, TextReader, etc.. do not have any scaling functionality so the scaling in the Clip still needs to be applied.

The new code adds a check to see if the scale type is SCALE_NONE and the reader type is QtImageReader or FFmpegReader. If so, it skips the scaling step.

The code resolves the issue but not sure if this is the best approach or the code is particularly clean. It might highlight the issue and give clues to where this could be improved.

[jeffski]

Note that the Clip::Reader() setter also needs to set the reader_type, but every time I tried to run this the QtImageReader or FFmpegReader passed to the clip is replaced by a FrameMapper by the time the scaling occurs so the image/video is always scaled twice. See: https://github.com/OpenShot/libopenshot/pull/797/files#diff-915ea951ee9b3b1e674b479f5b0f014be1a80f95268a021cbac74906d70c4518R270

[codecov]

Codecov Report

Merging #797 (54d7562) into develop (3c8dc71) will increase coverage by 0.02%.
The diff coverage is 94.11%.

@@             Coverage Diff             @@
##           develop     #797      +/-   ##
===========================================
+ Coverage    48.97%   49.00%   +0.02%     
===========================================
  Files          184      184              
  Lines        15702    15712      +10     
===========================================
+ Hits          7690     7699       +9     
- Misses        8012     8013       +1     

Impacted Files	Coverage Δ
src/Clip.h	62.50% <ø> (ø)
src/Clip.cpp	44.50% <94.11%> (+0.58%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c8dc71...54d7562. Read the comment docs.

[ferdnyc]

*sigh*

There is so much scaling littered throughout the code.

For the record, I personally believe much of the ad-hoc scaling is unnecessary, even misguided. (Particularly in FFmpegReader, which employs FFmpeg's swscale algorithms — algorithms designed and geared towards the scaling of AVFrame data, but clunky when used to scale images that will be extracted into QImage — and does so at a point in the pipeline when it is far too early to possibly make reliably accurate assumptions regarding the image dimensions required.)

drawImage() scaling

You know QPainter will also scale the image? Says so right in the docs. in fact Qt finally got fed up beating around the bush and gave that fact the blue-background callout treatment, in their most recent documentation update:

Arguably, we shouldn't be scaling anything ourselves (especially when it requires allocating a new copy of the image), we should just be computing the correct target rectangle within the output image and painting into it. QPainter will do the scaling in-place as part of the painting process, and probably do it far more efficiently. I feel like Qt's documentation keeps edging closer and closer to just beating us right over the head with that information, probably because we clearly aren't listening.

Say it with me now... "(for performance reasons)"

libopenshot's in-pipeline scaling digressions are reliably accompanied by comments declaring what a performance boost it's achieving. I'm not saying I think the arguments those comments make are wrong, I just think they're out of date. Their repetitive nature is a sign of rampant cargo-cultism at work.

I'm sure at one point, years ago, the effects of early scaling were measured and it did result in a significant performance boost. But since then, that same code (and the same comments alongside it) appears to have been blindly copy-pasted all over the library with minimal reexamination of the original claim. And when something is removed from its original context, all claims about its effects or benefits are no longer applicable unless they're re-validated, or at least re_evaluated_. The context is further shifted by however many years of technology evolution have gone by since those original claims, which may be less applicable in the context of more modern codecs, formats, and hardware.

While there are probably significant benefits to judiciously scaling once, I categorically reject the notion that scaling twice (or more!) ever, under any real-world circumstances, represents an unqualified net performance gain. To say nothing of the (entirely negative) effect on quality. Scaling is an expensive, generation-loss operation that shouldn't be scattered willy-nilly all over a codebase like so many handfuls of birdseed.

Preview rendering

There is one notable exception to my basic stance of, "We most likely don't need to, can't accurately and intelligently know how to, and unless we can show otherwise shouldn't be, performing ad-hoc scaling operations on frame data at random points while it's still moving through the pipeline". Things are very different when it comes to the OpenShot video preview.

Because the image has to be passed from C++ to Python, for that reason alone it's probably a good idea for it to be scaled on the C++ side. But even then, I'd argue that it's better if we do it as late in the process as possible. Ideally within the Qt video render code, just before it's embedded in the render signal that delivers it to the Python side.

Effects

Effects can also complicate matters here. When applying effects to the image as it's being prepared for output, it's likely that working on a scaled-down version of the image actually does perform noticeably better, possibly even enough to justify multiple intermediate scalings. After all, our effects are hardly a model of efficiency. But...

1. If we're prematurely scaling down the image to paper over performance issues with the effects, that's not the best sign,
2. Optimizing for that particular case — and in the process, penalizing the general case when effects aren't in play by forcing a loss of quality that may not even be necessary — may not align with users' own priorities,
3. And most of all, this just makes an even stronger case for the expansion of pre-rendering features in OpenShot's video preview, instead of trying to perform the entire Timeline compositing process frame-by-frame in real time for the preview display. Regardless how many optimizations we perform, or how many different points in the process we try to apply them, there will always be a Timeline complexity level that exceeds the system's ability to render it in real time.

@jeffski

Note that the Clip::Reader() setter also needs to set the reader_type, but every time I tried to run this the QtImageReader or FFmpegReader passed to the clip is replaced by a FrameMapper by the time the scaling occurs so the image/video is always scaled twice.

Not replaced, wrapped. The original Reader is still there, as the source Reader for the FrameMapper.

The wrapping can also be turned off, by calling Timeline::AutoMapClips(false). But it is enabled by default, and the expectation for the typical scenario is that it would be enabled. So things do get a bit messy, yeah.

In theory Clip could look "through" a FrameMapper set as its Reader source, to determine the Reader class of its Reader, but that feels less like cleaning up a mess than shoveling additional mess on top of it.

The new code adds a check to see if the scale type is SCALE_NONE and the reader type is QtImageReader or FFmpegReader. If so, it skips the scaling step.

Yeah, I don't love that, personally, for reasons I'm sure you've already predicted. (Possibly even share.) For starters, Clip is where scaling can actually be done semi-intelligently because now we have access to all of the applicable Keyframe properties, and can get their values for the specific frame being generated so we don't have to pull dumb shit like prematurely-scaling based on the maximum property values at some frame somewhere.

So given that, as you correctly point out, in the general case "the scaling in the Clip still needs to be applied", my preference would be to KEEP the Clip class's scaling code, unconditionally. And instead remove the messy, convoluted scaling code from the two Readers (out of... how many, now?) where (IMNSHO) it never belonged in the first place. (OK, maybe that's overreaching. Let's go with... "where it hasn't belonged for quite some time".)

I'd also much rather leave the Clip scaling to be done by QPainter directly, unless there are effects that need to be applied before drawImage() can be called.

[ferdnyc]

The other issue here, now that I think about it, is that SCALE_NONE is really kind of impossible in the general sense... or at least, it's totally incompatible with any sort of ad-hoc scaling. Because, SCALE_NONE doesn't really mean "DO NOT SCALE". It means, "start from a size that's based on the ratio between the input frame size and the "real" output frame size, instead of normalizing to the output dimensions (which is what all of the other scale modes do, just in different ways)".

There are two problems with that:

1. It means you have to know your output frame size up front, because if you change it the size of your SCALE_NONE inputs will change — ironically — relative to the output. (Unlike all of the other scaling modes. Those permit input sizes to adjust to any output size.)

2. Bigger issue, it means that when you're creating scaled-down representations of the output, you have to keep track of your "real" (or "intended") frame size separate from the size you're currently rendering to, which we don't do. If the output image is going to be scaled purely for display purposes, in certain circumstances, then all of the computations for positioning and sizing (not scaling) need to be performed based on the "real" output size, not the scaled output size. Then, everything can be scaled evenly from real-output to display-output dimensions, including SCALE_NONE frames. (Because otherwise, SCALE_NONE items would change their relative size every time the preview was resized, and nobody actually wants that.)

   We got rid of the global max-frame-size values in Settings, but that's not good enough. For SCALE_NONE to work correctly, we can't do preview scaling by changing the configured dimensions of the Timeline. It needs to know what dimensions the output is representing, even when it's generating scaled-down representations of that output. That third size is required as a common basis, once you have that all of the input sizes and output sizes can be computed relative to it. Otherwise they can't be fully reconciled, precisely because they're supposed to be independent of each other.

IMHO the "parent Clip", "parent Timeline" stuff in the Readers is still upside-down. It's not as bad as Settings::max_width / max_height was, but it's still overcomplicating the wrong things for the wrong reasons. If we want to retrieve scaled frames from Readers, then GetFrame() should take pixel dimensions and return frames with those exact dimensions. The caller is always in a much better position to determine what size the rendered frame needs to be, and then there's no need for complicated logic that involves chasing Clip or Timeline objects from the Reader in order to figure that stuff out.

• A SCALE_NONE Clip can pass the Reader's native dimensions back into it, and it'll get unscaled frames back.
• If the frames are being generated for preview output, the native dimensions can be scaled based on the preview scaling even when the Clip is SCALE_NONE.
• If aspect ratio should be preserved, pass in dimensions that have the same aspect ratio as the native ones. Otherwise, the Frame will be distorted.
• Just like QPainter, GetFrame(..., x, y)'s scaling logic only needs to apply one simple rule:

The dimension-accepting GetFrame() can also be an overload. The version that doesn't take pixel dimensions would then always produce unscaled, native-size Frames. (That gets tricky only for SVG, although all of our own SVGs currently do have a native size of 1920px × 1080px so absent other dimensions they can render to that size.)

I still feel like that would work out better for all libopenshot users — including OpenShot, which mostly wouldn't really have to worry about it anyway. It's Clip and Timeline that would internally need to pass dimensions to GetFrame(). They'd compute them by taking the native dimensions and adjusting based on the relevant property values and other parameters.

OpenShot would just need to supply Timeline with both its target profile dimensions (the "real" output size), and either its current preview dimensions, or just a single floating-point multiplier for the real dimensions. Either of the latter options would be updated whenever the preview resizes, but the former wouldn't. Currently Timeline doesn't know its true output dimensions unless it's being used for export. Otherwise it's being lied to and fed only the preview dimensions, which makes it impossible to correctly handle SCALE_NONE.

[JacksonRG]

@jeffski When I pulled this code and tested with it, I wasn't seeing a difference when I loaded an image and scaled it under different settings, and compared that to the develop branch.

Here's what I found:

• Scale = Best Fit, scale_x= 0.5, scale_y= 1
  
  • (Best Fit, Scale, and Crop All looked the same)
• Scale = None, scale_x= 0.5, scale_y= 1
  

[jeffski]

@JacksonRG - Is that a QtImageReader handling that image? Does it work with an FfmpegReader video file>? I checked ImageReader and it doesn't seem to have any scaling going on. Only QtImageReader and FfmpegReader are currently skipped from the 2nd scale as other Readers need it.

I'm nor familiar with the front end, just the library.

[jeffski]

Thanks @ferdnyc for your extensive analysis. Sounds like an extensive task to try and refactor and optimise all of that.

[JacksonRG]

@jeffski Oops, I'll put a cout in the files you changed, to make sure the files I'm using are calling the code you changed

[JacksonRG]

@jeffski I took another stab at testing this. Taking screenshot at a list of scale settings (none, x, y, and both). and with both scale:best-fit and scale:none. I'm not seeing a difference between the photos. Let me know if you have testing instructions on this PR. (something I'm getting in the habit of including in my own PRs)

[jeffski]

Just having another look at this issue as this patch seems to introduce a new issue anyway - for some reason when scale is set to none but you set scale_x and scale_y >1, then a video (FFmpegReader) still scales to 1, i.e. if you set scale to 2 or 3, it still renders at 1x.

@JacksonRG - I'm not familiar with how the OpenShot GUI works as I only use the library, but here is some code to recreate and test the issue using just the library:

        Clip clip1(new QtImageReader(TEST_IMAGE)); // path to test image
	clip1.Position(0);
	clip1.Layer(1);
	clip1.Start(0);
	clip1.End(4);
	clip1.scale = openshot::SCALE_NONE;
	clip1.scale_x.AddPoint(1); // try 0.5, 1, 2, etc...
	clip1.scale_y.AddPoint(1); // try 0.5, 1, 2, etc...

	Clip clip2(new FFmpegReader(TEST_VIDEO)); // path to test video
	clip2.Position(0.5);
	clip2.Layer(1);
	clip2.Start(4);
	clip2.End(8);
	clip2.scale = openshot::SCALE_NONE;
	clip2.scale_x.AddPoint(0.5); // try 0.5, 1, 2, etc...
	clip2.scale_y.AddPoint(0.5); // try 0.5, 1, 2, etc...

	Timeline timeline(1920, 1080, Fraction(25,1), 44100, 2, LAYOUT_STEREO);
	timeline.AddClip(&clip1);
	timeline.AddClip(&clip2);
	timeline.Open();

	timeline.color = openshot::Color("#ffffff");

	FFmpegWriter writer("/tmp/output.mp4");
	writer.SetAudioOptions(true, "aac", 44100, 2, LAYOUT_STEREO, 128000);
	writer.SetVideoOptions(true, "libx264", Fraction(24000, 1001), 1920, 1080, Fraction(1,1), false, false, 0);
	writer.PrepareStreams();
	writer.SetOption(VIDEO_STREAM, "crf", "23" );
	writer.WriteHeader();
	writer.Open();
	writer.WriteFrame(&timeline, 1, 8 * 25);

	writer.Close();
	timeline.Close();

[github-actions]

Merge conflicts have been detected on this PR, please resolve.