Set clip in constructor to resolve scale crop issue

[jeffski]

This fix resolves issue: #419

Clips are scale cropped in two places FFmpegReader.cpp and in Timeline.cpp:556.

FFmpegReader calculates the wrong size for scale with crop. It is because Timeline doesn't set clip for FFmpegReader (and for all readers) so FFmpegReader doesn't now anything about SCALE_CROP, because it is a property of Clip.

And in FFmpegReader in line 1396 (in the last version) Clip *parent = (Clip *) GetClip();

We get parent = NULL and so it doesn't check scale property and calculate sizes using default formulas which are incorrect for SCALE_CROP case (and may be for SCALE_FIT and SCALE_STRETCH too).

I'll post some before and after screen-grabs just now but this change fixes the issue with 1:1 video scaling/cropping.

[codecov-io]

Codecov Report

Merging #513 into develop will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop     #513      +/-   ##
===========================================
- Coverage    48.25%   48.24%   -0.01%     
===========================================
  Files          128      128              
  Lines         9954     9956       +2     
===========================================
  Hits          4803     4803              
- Misses        5151     5153       +2     

Impacted Files	Coverage Δ
src/Clip.cpp	39.60% <0.00%> (-0.16%)	⬇️

---

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 6e7ad23...4e6c181. Read the comment docs.

[jeffski]

Here is before and after example.

You can clearly see the vertical lines of the trees in the before example are pixelated and have a 'wavey' kind of effects going on. The after example is crisp and sharp.

Before: https://shillitto.s3-ap-southeast-2.amazonaws.com/openshot/before.mp4
After: https://shillitto.s3-ap-southeast-2.amazonaws.com/openshot/after.mp4

[ferdnyc]

Hmm. My only major concern with this, on first glance, would be all of the times OpenShot (and possibly libopenshot too) does something like this:

f = File(somepath)
throwaway = Clip(somepath)
dostuffwith(throwaway.Reader())

Which leaves me concerned that we could end up with reader data that references nonexistent clips, because the reader's parent was disposed of. I'm not SURE it's a problem, it's just my worry.

... Though I suppose at least in OpenShot, since the parent ref is a pointer it likely wouldn't be exposed from the Python bindings. So this may not be any kind of issue really.

Great catch, though! Something's definitely out of whack, there. If setting the parent fixes it, and no other issues turn up, maybe we just do that. Clean & simple.

Looking at that part of the code, though, I have to wonder how scaling everything TWICE can actually be any sort of performance improvement (as the comments there claim).

I get that it's intended to keep memory consumption and cache sizes down, but the fact remains that we're scaling INPUT data that's very likely just going to be scaled again for OUTPUT. And as you discovered, we're doing it indiscriminately, even in situations where we can't tell whether it makes sense or not — which is also hurting quality. Dubious benefits there, if you ask me.

Now I'm really curious to see what a libopenshot build that removed all that input scaling would look like, memory/performance-wise.

[SuslikV]

It seems that it changes nothing in openshot-qt Export.

[ferdnyc]

Here is before and after example.

You can clearly see the vertical lines of the trees in the before example are pixelated and have a 'wavey' kind of effects going on. The after example is crisp and sharp.

Wow! That is pretty much night-and-day, agreed. To the extent that my strong inclination is just to merge this ASAP and deal with any possible fallout afterwards.

[ferdnyc]

It seems that it changes nothing in openshot-qt Export.

I suspect that's because OpenShot only uses the openshot::Clip::Clip(std::string path) constructor, which allocates the reader itself. This change would only affect cases where an existing reader is used to create a new openshot::Clip.

(And therefore, arguably, maybe it's the caller's job to call reader->SetClip()? I wonder if there are any possible cases where you'd want to construct a clip with an existing openshot::Reader, but have it not reparented? After all, if the caller really does want it reparented, they could be calling SetClip() themselves. ...Then again, openshot::Clip::Reader() does always call reader->SetClip(), so the API seems internally conflicted on this point. And it really shouldn't be the caller's worry. I'd tend to agree that if openshot::Clip::Reader() calls reader->SetClip(), then openshot::Clip::Clip(reader) should as well.)

The odd thing is, when Clip(path) allocates a new reader for itself, it also doesn't call SetClip() on the new reader to set itself as the parent. Seems like it should be doing that there, as well. Let me take a quick look at ReaderBase to see what's expected there.

[jeffski]

We are just taking a look at optimising the two places where the resizing is done, to remove or only run the resizing if needed. Give us a day or two and will push an update.

[ferdnyc]

Hmm. Nothing of use. parent was created relatively recently (a year and a half ago), in one of @jonoomph 's wide-ranging megacommits. (7b6eb9c)

At the time, setting it was added to openshot::Clip::Reader(), and to openshot::Clip::SetJsonValue() (which is most likely why OpenShot is covered). However, despite its name, it doesn't appear to indicate ownership of the Clip object in any way — that's what the allocated_reader pointer is for. And it does appear to be used only for the purpose of determining input scaling. A bunch of code that used to live in openshot::Clip was moved (in slightly different forms) to openshot::QtImageReader and openshot::FFmpegReader instead, which is why it now needs to know its parent in order to look up the necessary data.

The else clause on all of that ­— used when the clip is set SCALE_NONE — used to be:

else{
    // No scaling, use original image size (slower)
    reader->SetMaxSize(0, 0);
}

but now, not only does the SCALE_NONE path contain:

else {
    // No scaling, use original image size (slower)
    max_width = info.width;
    max_height = info.height;
}

(...Come to think of it, the fact that this now lives in FFmpegReader probably explains the scaling bug with disabled clips, and why they had to be set SCALE_NONE to stay properly invisible.) Anyway, because all of that is wrapped in if (parent), when there's no parent the code above all of that will apply, which is:

libopenshot/src/FFmpegReader.cpp

Lines 1289 to 1294 in 4e6c181

	int max_width = openshot::Settings::Instance()->MAX_WIDTH;
	if (max_width <= 0)
	max_width = info.width;
	int max_height = openshot::Settings::Instance()->MAX_HEIGHT;
	if (max_height <= 0)
	max_height = info.height;

What happens will be wholly dependent on the openshot::Settings values, because what immediately follows this insertion into FFmpegReader remained the same:

libopenshot/src/FFmpegReader.cpp

Lines 931 to 948 in 7b6eb9c

	// Determine if image needs to be scaled (for performance reasons)
	int original_height = height;
	if (max_width != 0 && max_height != 0 && max_width < width && max_height < height) {
	// Override width and height (but maintain aspect ratio)
	float ratio = float(width) / float(height);
	int possible_width = round(max_height * ratio);
	int possible_height = round(max_width / ratio);
	if (possible_width <= max_width) {
	// use calculated width, and max_height
	width = possible_width;
	height = max_height;
	} else {
	// use max_width, and calculated height
	width = max_width;
	height = possible_height;
	}
	}

When the code lived in Clip there were no defaults for max_width and max_height, I assume they always initialized to 0. But now that they're never going to be 0 (in addition to those != 0 tests being pointless), the code does very different things.

[ferdnyc]

This scaling code is nutso, anyway. Like, it uses QSize (only) for SCALE_CROP now, but it uses it all crazy.

Want to know how to implement SCALE_FIT with QSize?

auto max = QSize(max_width, max_height);
auto frame = QSize(info.width, info.height);
auto target = frame.scaled(max, Qt::KeepAspectRatio);

Done. Want to know how to implement SCALE_CROP?

auto target = frame.scaled(max, Qt::KeepAspectRatioByExpanding);

[ferdnyc]

As far as SCALE_STRETCH, currently in the code it uses the SCALE_FIT scaling, but shouldn't it use SCALE_CROP instead?

Like, imagine you're scaling down a video at a different aspect ratio, using SCALE_STRETCH. Currently because it uses the SCALE_FIT sizing, the video will be scaled to the size of the frame, with one dimension being smaller than the frame (because the aspect ratio is preserved). Then later it'll be expanded in that dimension (losing resolution), to make it fill the frame.

But if it were using the SCALE_CROP sizing, then the video would be the same resolution as the frame, in the stretch direction. It'd need to be scaled down in the other dimension. That should result in a higher-quality SCALE_STRETCH... right? Somebody please sanity-check my math on that, before I start on a PR to change all this. (Though I'll probably wait for @jeffski 's update anyway, so I can change it only where necessary.)

[codecov-commenter]

Codecov Report

Merging #513 into develop will increase coverage by 0.07%.
The diff coverage is 25.00%.

@@             Coverage Diff             @@
##           develop     #513      +/-   ##
===========================================
+ Coverage    48.25%   48.32%   +0.07%     
===========================================
  Files          128      128              
  Lines         9954     9953       -1     
===========================================
+ Hits          4803     4810       +7     
+ Misses        5151     5143       -8     

Impacted Files	Coverage Δ
src/Clip.cpp	39.72% <25.00%> (-0.04%)	⬇️
include/FFmpegUtilities.h	100.00% <0.00%> (ø)
src/FFmpegReader.cpp	68.79% <0.00%> (+0.46%)	⬆️
src/QtImageReader.cpp	61.32% <0.00%> (+4.71%)	⬆️

---

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 6e7ad23...5b5950c. Read the comment docs.

[jeffski]

I misunderstood my co-developer regarding more changes to skip one of the scaling steps so please consider the pull request complete unless you have more change requests. We were wondering if scaling would be faster/more efficient using FFMPEG instead of Qt or if there is a reason for using Qt as well.

More than happy for you to pick this up and optimise/fix further.

[jeffski]

A related issue I had a while back was using SCALE_NONE - I was trying to use a logo as a watermark and the logo was already scaled i.e. 300px x 200px - using SCALE_NONE would still stretch it to the width/height of the viewport. My expectation would be that it would remain 300px x 200px. I think there is comments in the code saying that needed fixed.

[ferdnyc]

A related issue I had a while back was using SCALE_NONE - I was trying to use a logo as a watermark and the logo was already scaled i.e. 300px x 200px - using SCALE_NONE would still stretch it to the width/height of the viewport. My expectation would be that it would remain 300px x 200px. I think there is comments in the code saying that needed fixed.

Yeah, I would... very much expect so. SCALE_NONE should be, you know, unscaled.

What format was the file? You say "a logo", but was it an SVG file, or a bitmap image? I wouldn't be surprised if the SVG rendering always renders to the timeline frame size. (You gave the dimensions in px so it was probably a bitmap, but if so I thought that worked, at least for video. Unless maybe SCALE_NONE works right for video, but not images.)

...I guess a workaround would be to matte the watermark onto a transparent PNG the size of the frame, so it doesn't need to be scaled. (You no doubt thought of that.) But, yeah, it'd be cool if that just worked right as-is.

[ferdnyc]

I'll let this sit for a little bit to see if anyone else takes an interest, but like I said my inclination is to merge it as quickly as possible given the clear benefits seen in those before/after videos. I don't believe it'll affect OpenShot itself anyway, the existing code was already doing the right things in the APIs it uses. I think the others were just overlooked.

[SuslikV]

A related issue I had a while back was using SCALE_NONE - I was trying to use a logo as a watermark and the logo was already scaled i.e. 300px x 200px - using SCALE_NONE would still stretch it to the width/height of the viewport. My expectation would be that it would remain 300px x 200px. I think there is comments in the code saying that needed fixed.

In OpenShot it works OK. No noticeable issues with the Scale set to None. At least, in now days.

[ferdnyc]

No complaints, so in it goes. Thanks @jeffski !

[jeffski]

I just checked this vs the old code and it seems to fix the SCALE_NONE none issue I was having. With the old code a 400px x 103px bitmap image was stretched to the width of the video (1280px). With the new code the image is the correct size when rendered at 400px x 103px. So two birds with one stone.