CSS: add support for min-width, max-width, min-height, max-height #409

poire-z · 2021-01-18T10:05:31Z

CSS: parse and store min/max-width/height

Increase nb of bitmaps from 2 to 3 lUInt32 as we're going over 64 properties.

Add getStyledImageSize() ensuring min/max-width/height

Compute image size according to CSS2 specs, ensuring CSS width, height, min-width, min-height, max-width and max-height.
Used in lvtextfm when drawing images, and when estimating block widths in getRenderedWidths() instead of the previous wrong computation.
Limitation: heights in % are not supported and ignored.

renderBlockElementEnhanced(): ensure min/max-width/height

Ensure min-width, min-height, max-width (but not max-height) on block elements, inline-block/table, block floats and floating images (min-height in % not supported).
Ensure min-width on the table element itself (but not supported on the table inner elements like cells and cols).

Also includes:
EPUB ncx TOC: allow items with an empty title
No reason to ignore them - and they may have sub-items with non-empty titles that would all be ignored. koreader/koreader#5961 (comment)

resizeImage(): restore original scaling options code

It was hardcoded'ly removed as part of #35 9214bfc when implementing support for CSS width & height for images, so they don't get in the way.
Removed the hardcoding so the original CoolReader settings can work again. buggins/coolreader#125 (comment)
We can still disable that from frontend code with:

-- Disable crengine image scaling options (we prefer scaling them via crengine.render.dpi)
self._document:setIntProperty("crengine.image.scaling.zoomin.block.mode", 0)
self._document:setIntProperty("crengine.image.scaling.zoomin.block.scale", 1)
self._document:setIntProperty("crengine.image.scaling.zoomin.inline.mode", 0)
self._document:setIntProperty("crengine.image.scaling.zoomin.inline.scale", 1)
self._document:setIntProperty("crengine.image.scaling.zoomout.block.mode", 0)
self._document:setIntProperty("crengine.image.scaling.zoomout.block.scale", 1)
self._document:setIntProperty("crengine.image.scaling.zoomout.inline.mode", 0)
self._document:setIntProperty("crengine.image.scaling.zoomout.inline.scale", 1)

This change is

It was hardcoded'ly removed as part of 9214bfc when implementing support for CSS width & height for images, so they don't get in the way. Removed the hardcoding so the original CoolReader settings can work again. We can still disable that from frontend code.

Increase nb of bitmaps from 2 to 3 lUInt32 as we're going over 64 properties.

Compute image size according to CSS2 specs, ensuring CSS width, height, min-width, min-height, max-width and max-height. Used in lvtextfm when drawing images, and when estimating block widths in getRenderedWidths() instead of the previous wrong computation. Limitation: heights in % are not supported and ignored.

Ensure min-width, min-height, max-width (but not max-height) on block elements, inline-block/table, block floats and floating images (min-height in % not supported). Ensure min-width on the table element itself (but not supported on the table inner elements like cells and cols).

NiLuJe

Nothing jumps out at a cursory glance, but I'm obviously unfamiliar with the logic involved ;).

Frenzie

Nothing much to say about it either.

poire-z · 2021-02-14T12:21:08Z

Possible rounding error (ceiling while we might want flooring) somewhere, causing some image to be splitted onto 2 pages (because after resizing, it is higher than the page height) instead of make a page on its own:

PS: AddLine (7ea65498, #70): 2799 > 2840 (0)
PS: AddLine (7ea65498, #71): 2840 > 2881 (8)
PS: AddLine (7ea65498, #72): 2881 > 2922 (0)
PS: AddLine (7ea65498, #73): 2922 > 2933 (1024)
PS: AddLine (7ea65498, #74): 2933 > 2959 (0)
m_max_img_height: 1343 (page height: 1353)        <= max image height = 1343
getStyledImageSize: w=771 h=1398
resizeImage: w=741 h=1344     <= WRONG, should be h=1343
PS: AddLine (7ea65498, #75): 2959 > 4313 (0)
PS: splitting lines into pages, page height=1353
PS: Adding line 0>0 h=0, flags=<2|1>   starting page with it
PS:           new current page 0>0 h=0
[...]
PS: Adding line 2881>2922 h=41, flags=<0|0>   to current page 2635>2881 h=246
PS:   fit, split to avoid
PS: Adding line 2922>2933 h=11, flags=<0|0>   to current page 2635>2922 h=287
PS:   fit, split allowed
PS: Adding line 2933>2959 h=26, flags=<0|0>   to current page 2635>2933 h=298
PS:   fit, split allowed
PS: Adding line 2959>4313 h=1354, flags=<0|0>   to current page 2635>2959 h=324
PS:     line overflows page: 1678 > 1353
PS: ==== SPLITTED AS PAGE 2: 2635 > 3988 h=1353
PS:           new current page 3988>4313 h=325
PS: ========= ADDING PAGE 3: 3988 > 4313 h=325 [rtl l:0/1 fl:0/0]

poire-z · 2021-02-14T14:37:27Z

I could have get done with adding at the end:

        // Really be sure the computations did not overflow
        if ( height > max_h )
            height = max_h;
        if ( width > max_w )
            width = max_w;

But I think I'm gonna trust buggins/coolreader#252 (without the roundi(), as the above case proves we're safer with flooring) to solve this - just tried it and it gives correct values. It's just easier to understand than the current code.

Current code:

        if ( maxScaleMult<1 ) maxScaleMult = 1;
        if ( arbitraryImageScaling ) {
            int pscale_x = 1000 * maxw / width;
            int pscale_y = 1000 * maxh / height;
            int pscale = pscale_x < pscale_y ? pscale_x : pscale_y;
            int maxscale = maxScaleMult * 1000;
            if ( pscale>maxscale )
                pscale = maxscale;
            height = height * pscale / 1000;
            width = width * pscale / 1000;
        } else {
            if (maxw == 0 || maxh == 0) {
                // Avoid a floating point exception (division by zero) crash.
                printf("CRE WARNING: resizeImage(maxw=0 or maxh=0)\n");
                return;
            }
            int scale_div = 1;
            int scale_mul = 1;
            int div_x = (width * 1000 / maxw);
            int div_y = (height * 1000 / maxh);
            if ( maxScaleMult>=3 && height*3 < maxh - 20
                    && width*3 < maxw - 20 ) {
                scale_mul = 3;
            } else if ( maxScaleMult>=2 && height * 2 < maxh - 20
                    && width * 2 < maxw - 20 ) {
                scale_mul = 2;
            } else if (div_x>1 || div_y>1) {
                if (div_x>div_y)
                    scale_div = div_x;
                else
                    scale_div = div_y;
            }
            height = height * 1000 * scale_mul / scale_div;
            width = width * 1000 * scale_mul / scale_div;
        }

Could become (buggins/coolreader#252, without roundi and the latest else made clearer):

        if ( maxScaleMult < 1 ) maxScaleMult = 1;

        if ( maxw < 2 ) maxw = 2;
        if ( maxh < 2 ) maxh = 2;

        if ( !arbitraryImageScaling ) {
            // Integer scaling. Try some integer scales first.
            if ( maxScaleMult >= 3 && height*3 < maxh - 20 && width*3 < maxw - 20 ) {
                width = width * 3;
                height = height * 3;
                return;
            }
            if ( maxScaleMult >= 2 && height * 2 < maxh - 20 && width * 2 < maxw - 20 ) {
                width = width * 2;
                height = height * 2;
                return;
            }
            if ( height <= maxh && width <= maxw ) {
                // Fits without scaling (one).
                return;
            }
            // Fall through to arbitrary scale down.
        }

        if (width * maxh > height * maxw) {
            if (width * maxScaleMult > maxw) {
                height = height * maxw / width;
                width = maxw;
                return;
            }
        }
        else {
            if (height * maxScaleMult > maxh) {
                width = width * maxh / height;
                height = maxh;
                return;
            }
        }
        width = width * maxScaleMult;
        height = height * maxScaleMult;

I'm still not friend with aspect ratio and these width * maxh > height * maxw kind of thinking :/
Pinging @NiLuJe for a quick review that this is as fine as it looks simple :)

NiLuJe · 2021-02-14T17:11:33Z

I wouldn't mind an explanation behind the magic 20 pixels of scratch in the high (up)scale ratio checks ;).

I also wouldn't mind better variables names, because I have no idea what's the source image's dimensions, what's the scaled values (because it appears that the same out pointer is used for both...). At least I imagine maxh/maxw and are the bounding box dimensions.

Otherwise, AR is as simple as that (which translates to this with slightly less obscure variable names).
But, basically, in Qt's case: wd/ht are the image's width/height, the s. stuff is the requested scaled values, and the rw/rh are the computed rescaled values after AR coercion.

Sidebar: I wouldn't call CRe's house style "clearer" ^^. It's just... CRe's house style ;p. But we've already had this conversation ^^.

poire-z · 2021-02-14T17:23:47Z

I wouldn't mind an explanation behind the magic 20 pixels of scratch in the high (up)scale ratio checks ;).

I have no explanation: it's there since ages, not from me, and it's in a code path we won't be taking (but CoolReader may).

I also wouldn't mind better variables names, because I have no idea what's the source image's dimensions, what's the scaled values (because it appears that the same out pointer is used for both...). At least I imagine maxh/maxw and are the bounding box dimensions.

Right, computed (from CSS or native image size) width & height, updated in place:
void resizeImage( int & width, int & height, int maxw, int maxh, bool arbitraryImageScaling, int maxScaleMult )
With KOReader, arbitraryImageScaling=false, maxScaleMult=1.

Otherwise, AR is as simple as that (which translates to this with slightly less obscure variable names).
But, basically, in Qt's case: wd/ht are the image's width/height, the s. stuff is the requested scaled values, and the rw/rh are the computed rescaled values after AR coercion.

Oh my, why did I ask ?! :)
Don't want to rethink about it, @ourairquality did the thinking :) and it looks like what we did in the min/max-width/height code, Just wanted another confirmation :)

NiLuJe · 2021-02-14T17:23:49Z

Also, this appears to prefer scaling to fit dims * maxScaleMult, instead of fitting maxw/maxh, while, AFAICT, the older code properly used it as a cap.

NiLuJe · 2021-02-14T17:44:15Z

Which means, if I understood what maxScaleMult is supposed to do, the tail end could look something like this:

// FIXME: Move me to a dedicated inline function to avoid code dupe inside the maxScaleMult block
int bbox_width = maxw;
int bbox_height = maxh;
// We want to fit *inside* the bbox
int scaled_width = bbox_width;
int scaled_height = bbox_height;

int rescaled_width = bbox_height * width / height;
if (rescaled_width <= bbox_width) {
	scaled_width = rescaled_width;
} else {
	scaled_height = bbox_width * height / width;
}

// We also want to make sure we haven't gone past maxScaleMult when upscaling...
// FIXME: Should we skip this if maxScaleMult is 1 or something?
if (scaled_height > height && scaled_height / height > maxScaleMult || scaled_width > width && scaled_width / width > maxScaleMult) {
	bbox_width = width * maxScaleMult;
	bbox_height = height * maxScaleMult;
	
        // e.g., call inlined function here ;p
	scaled_width = bbox_width;
	scaled_height = bbox_height;

	rescaled_width = bbox_height * width / height;
	if (rescaled_width <= bbox_width) {
		scaled_width = rescaled_width;
	} else {
		scaled_height = bbox_width * height / width;
	}
}

// We're done, update out pointers
width = scaled_width;
height = scaled_height;

NiLuJe · 2021-02-14T17:59:11Z

e.g.,

static inline void fit_inside(int & width, int & height, int bbox_width, int bbox_height) {
	// We want to fit *inside* the bbox, so this is a good start, we'll honor AR below.
	int scaled_width = bbox_width;
	int scaled_height = bbox_height;
	
	int rescaled_width = bbox_height * width / height;
	if (rescaled_width <= bbox_width) {
		scaled_width = rescaled_width;
	} else {
		scaled_height = bbox_width * height / width;
	}
	
	// We're done, update out pointers
	width = scaled_width;
	height = scaled_height;
}

...

// Start with the source dimensions, fit_inside will update 'em
int scaled_width = width;
int scaled_height = height;
fit_inside(scaled_width, scaled_height, maxw, maxh);

// We also want to make sure we haven't gone past maxScaleMult when upscaling...
// FIXME: Should we skip this if maxScaleMult is 1 or something?
if (scaled_height > height && scaled_height / height > maxScaleMult || scaled_width > width && scaled_width / width > maxScaleMult) {
        // Start with the source dimensions, fit_inside will update 'em
	scaled_width = width;
	scaled_height = height;
	fit_inside(scaled_width, scaled_height, width * maxScaleMult, height * maxScaleMult);
}

// We're done, update out pointers
width = scaled_width;
height = scaled_height;

poire-z · 2021-02-14T18:14:09Z

Mhhh, hard to get into what you want to do (just as hard to get into the current code, and the new one :) but it looks like you want to always scale to fit in the bbox - which is not the only purpose of this function: when arbitraryImageScaling=false, maxScaleMult=1, it should just downscale if needed, not try to fit the bbox. You also have no arbitraryImageScaling=false code path.

From what I understand, this function should do:

when arbitraryImageScaling=false, try multiply by maxScaleMult=3, then 2, then 1 until it fits in the bbox. When it does, done. If it does not, scale down to fit.
when arbitraryImageScaling=true, scale up to fit, but not going further than maxScaleMult. If it fits, done, if it does not, scale down to fit.
in our KOReader case, arbitraryImageScaling=false, maxScaleMult=1, it should just not increase the image, and just ensure we don't overflow maxh/maxw by scaling it down to fit.

NiLuJe · 2021-02-14T18:23:44Z

Oh, that's what I meant by "the tail end of" (hence the ellipsis), e.g., it's just the arbitraryImageScaling == true bit of the "new" code.
The earlier bit looks sane-ish, if you forget about those magic 20 pixels ^^.

e.g., it replaces this section, but keeps the rest:

        if (width * maxh > height * maxw) {
            if (width * maxScaleMult > maxw) {
                height = height * maxw / width;
                width = maxw;
                return;
            }
        }
        else {
            if (height * maxScaleMult > maxh) {
                width = width * maxh / height;
                height = maxh;
                return;
            }
        }
        width = width * maxScaleMult;
        height = height * maxScaleMult;

NiLuJe · 2021-02-14T18:26:47Z

* when arbitraryImageScaling=true, scale up to fit, but not going further than maxScaleMult. If it fits, done, if it does not, scale down to fit.

So, yeah, that's what I understood to, and I think that the new code broke this, hence my revision.

poire-z · 2021-02-14T18:46:50Z

this appears to prefer scaling to fit dims * maxScaleMult, instead of fitting maxw/maxh, while, AFAICT, the older code properly used it as a cap.

I think that the new code broke this

After I read that above, and re-read the new code, I had the same feeling as you did: we might do x1.5 if 2 does not fit.
But may be our feeling was wrong: in its first branch, it tries 3, 2, then 1. if 1 fits (if ( height <= maxh && width <= maxw ) { return; }): no scaling. If 1 does not fit, we're down scaling.

Pfff, you're talking about the other codepath.
I still think the new code does that correctly: it first checks if using maxScaleMult would overflow, and if it does it cap it to maxw/maxh (ok, doing it a bit not the natural way: checking which of w or h might overflow first with if (width * maxh > height * maxw) {, then * maxScaleMult that one, and it it overflows, cap it. If not, fallback to last part to * maxScaleMult everything.

(Oh, why do we have to spend so much time on these kind of things...;)

NiLuJe · 2021-02-14T18:52:37Z

Yeah, the convoluted way in which it handles that is precisely what makes me wary of the effective results ;).

(As in, I literally can't wrap my brain around it, so I'm not confident it does it right without putting it through a rigorous test harness, which we don't have ^^. While I feel more confident about an approach that actually compares a scale ratio post-scaling, which is also what the current code does ^^.)

poire-z · 2021-02-14T23:55:25Z

putting it through a rigorous test harness, which we don't have

Dunno if that's rigorous enough: testResizeImage.cpp.txt

Haven't extended the test case to limit the output below.
Most are identical.
For the -1/+1, "new" is better.
For the very large diff, it's obvious "current" has some bugs.
For the not too large diff, it seems "new" does it more like I understand the option should work - but also that it might have a bug with arbSc 0 scMul 2 not scaling up although it should - but a bit too tired to really put my mind on it :)

1,2c3,4
> [22 44] arbSc 0 scMul 1 : 22/44 => old: 22/44 |new: 22/44
> [22 44] arbSc 0 scMul 2 : 22/44 => old: 22/44 |new: 22/44
> [22 44] arbSc 0 scMul 3 : 22/44 => old: 22/44 |new: 22/44
> [22 44] arbSc 0 scMul 4 : 22/44 => old: 22/44 |new: 22/44
> [22 44] arbSc 1 scMul 1 : 22/44 => old: 22/44 |new: 22/44
> [22 44] arbSc 1 scMul 2 : 22/44 => old: 22/44 |new: 22/44
> [22 44] arbSc 1 scMul 3 : 22/44 => old: 22/44 |new: 22/44
> [22 44] arbSc 1 scMul 4 : 22/44 => old: 22/44 |new: 22/44
> [22 44] arbSc 0 scMul 1 : 44/22 => old: 22/11 |new: 22/11
> [22 44] arbSc 0 scMul 2 : 44/22 => old: 22/11 |new: 22/11
> [22 44] arbSc 0 scMul 3 : 44/22 => old: 22/11 |new: 22/11
> [22 44] arbSc 0 scMul 4 : 44/22 => old: 22/11 |new: 22/11
> [22 44] arbSc 1 scMul 1 : 44/22 => old: 22/11 |new: 22/11
> [22 44] arbSc 1 scMul 2 : 44/22 => old: 22/11 |new: 22/11
> [22 44] arbSc 1 scMul 3 : 44/22 => old: 22/11 |new: 22/11
> [22 44] arbSc 1 scMul 4 : 44/22 => old: 22/11 |new: 22/11
> [22 44] arbSc 0 scMul 1 : 100/200 => old: 22/44 |new: 22/44
> [22 44] arbSc 0 scMul 2 : 100/200 => old: 22/44 |new: 22/44
> [22 44] arbSc 0 scMul 3 : 100/200 => old: 22/44 |new: 22/44
> [22 44] arbSc 0 scMul 4 : 100/200 => old: 22/44 |new: 22/44
> [22 44] arbSc 1 scMul 1 : 100/200 => old: 22/44 |new: 22/44
> [22 44] arbSc 1 scMul 2 : 100/200 => old: 22/44 |new: 22/44
> [22 44] arbSc 1 scMul 3 : 100/200 => old: 22/44 |new: 22/44
> [22 44] arbSc 1 scMul 4 : 100/200 => old: 22/44 |new: 22/44
> [22 44] arbSc 0 scMul 1 : 150/100 => old: 22/14 |new: 22/14
> [22 44] arbSc 0 scMul 2 : 150/100 => old: 22/14 |new: 22/14
> [22 44] arbSc 0 scMul 3 : 150/100 => old: 22/14 |new: 22/14
> [22 44] arbSc 0 scMul 4 : 150/100 => old: 22/14 |new: 22/14
< [22 44] arbSc 1 scMul 1 : 150/100 => old: 21/14 |new: 22/14 diff 1 / 0
< [22 44] arbSc 1 scMul 2 : 150/100 => old: 21/14 |new: 22/14 diff 1 / 0
< [22 44] arbSc 1 scMul 3 : 150/100 => old: 21/14 |new: 22/14 diff 1 / 0
< [22 44] arbSc 1 scMul 4 : 150/100 => old: 21/14 |new: 22/14 diff 1 / 0
> [44 22] arbSc 0 scMul 1 : 22/44 => old: 11/22 |new: 11/22
> [44 22] arbSc 0 scMul 2 : 22/44 => old: 11/22 |new: 11/22
> [44 22] arbSc 0 scMul 3 : 22/44 => old: 11/22 |new: 11/22
> [44 22] arbSc 0 scMul 4 : 22/44 => old: 11/22 |new: 11/22
> [44 22] arbSc 1 scMul 1 : 22/44 => old: 11/22 |new: 11/22
> [44 22] arbSc 1 scMul 2 : 22/44 => old: 11/22 |new: 11/22
> [44 22] arbSc 1 scMul 3 : 22/44 => old: 11/22 |new: 11/22
> [44 22] arbSc 1 scMul 4 : 22/44 => old: 11/22 |new: 11/22
> [44 22] arbSc 0 scMul 1 : 44/22 => old: 44/22 |new: 44/22
> [44 22] arbSc 0 scMul 2 : 44/22 => old: 44/22 |new: 44/22
> [44 22] arbSc 0 scMul 3 : 44/22 => old: 44/22 |new: 44/22
> [44 22] arbSc 0 scMul 4 : 44/22 => old: 44/22 |new: 44/22
> [44 22] arbSc 1 scMul 1 : 44/22 => old: 44/22 |new: 44/22
> [44 22] arbSc 1 scMul 2 : 44/22 => old: 44/22 |new: 44/22
> [44 22] arbSc 1 scMul 3 : 44/22 => old: 44/22 |new: 44/22
> [44 22] arbSc 1 scMul 4 : 44/22 => old: 44/22 |new: 44/22
> [44 22] arbSc 0 scMul 1 : 100/200 => old: 11/22 |new: 11/22
> [44 22] arbSc 0 scMul 2 : 100/200 => old: 11/22 |new: 11/22
> [44 22] arbSc 0 scMul 3 : 100/200 => old: 11/22 |new: 11/22
> [44 22] arbSc 0 scMul 4 : 100/200 => old: 11/22 |new: 11/22
> [44 22] arbSc 1 scMul 1 : 100/200 => old: 11/22 |new: 11/22
> [44 22] arbSc 1 scMul 2 : 100/200 => old: 11/22 |new: 11/22
> [44 22] arbSc 1 scMul 3 : 100/200 => old: 11/22 |new: 11/22
> [44 22] arbSc 1 scMul 4 : 100/200 => old: 11/22 |new: 11/22
> [44 22] arbSc 0 scMul 1 : 150/100 => old: 33/22 |new: 33/22
> [44 22] arbSc 0 scMul 2 : 150/100 => old: 33/22 |new: 33/22
> [44 22] arbSc 0 scMul 3 : 150/100 => old: 33/22 |new: 33/22
> [44 22] arbSc 0 scMul 4 : 150/100 => old: 33/22 |new: 33/22
> [44 22] arbSc 1 scMul 1 : 150/100 => old: 33/22 |new: 33/22
> [44 22] arbSc 1 scMul 2 : 150/100 => old: 33/22 |new: 33/22
> [44 22] arbSc 1 scMul 3 : 150/100 => old: 33/22 |new: 33/22
> [44 22] arbSc 1 scMul 4 : 150/100 => old: 33/22 |new: 33/22
< [100 200] arbSc 0 scMul 1 : 22/44 => old: 100/200 |new: 22/44 diff -78 / -156
< [100 200] arbSc 0 scMul 2 : 22/44 => old: 44000/88000 |new: 44/88 diff -43956 / -87912
< [100 200] arbSc 0 scMul 3 : 22/44 => old: 66000/132000 |new: 66/132 diff -65934 / -131868
< [100 200] arbSc 0 scMul 4 : 22/44 => old: 66000/132000 |new: 66/132 diff -65934 / -131868
> [100 200] arbSc 1 scMul 1 : 22/44 => old: 22/44 |new: 22/44
> [100 200] arbSc 1 scMul 2 : 22/44 => old: 44/88 |new: 44/88
> [100 200] arbSc 1 scMul 3 : 22/44 => old: 66/132 |new: 66/132
> [100 200] arbSc 1 scMul 4 : 22/44 => old: 88/176 |new: 88/176
< [100 200] arbSc 0 scMul 1 : 44/22 => old: 100/50 |new: 44/22 diff -56 / -28
< [100 200] arbSc 0 scMul 2 : 44/22 => old: 100/50 |new: 44/22 diff -56 / -28
< [100 200] arbSc 0 scMul 3 : 44/22 => old: 100/50 |new: 44/22 diff -56 / -28
< [100 200] arbSc 0 scMul 4 : 44/22 => old: 100/50 |new: 44/22 diff -56 / -28
> [100 200] arbSc 1 scMul 1 : 44/22 => old: 44/22 |new: 44/22
> [100 200] arbSc 1 scMul 2 : 44/22 => old: 88/44 |new: 88/44
< [100 200] arbSc 1 scMul 3 : 44/22 => old: 99/49 |new: 100/50 diff 1 / 1
< [100 200] arbSc 1 scMul 4 : 44/22 => old: 99/49 |new: 100/50 diff 1 / 1
> [100 200] arbSc 0 scMul 1 : 100/200 => old: 100/200 |new: 100/200
> [100 200] arbSc 0 scMul 2 : 100/200 => old: 100/200 |new: 100/200
> [100 200] arbSc 0 scMul 3 : 100/200 => old: 100/200 |new: 100/200
> [100 200] arbSc 0 scMul 4 : 100/200 => old: 100/200 |new: 100/200
> [100 200] arbSc 1 scMul 1 : 100/200 => old: 100/200 |new: 100/200
> [100 200] arbSc 1 scMul 2 : 100/200 => old: 100/200 |new: 100/200
> [100 200] arbSc 1 scMul 3 : 100/200 => old: 100/200 |new: 100/200
> [100 200] arbSc 1 scMul 4 : 100/200 => old: 100/200 |new: 100/200
> [100 200] arbSc 0 scMul 1 : 150/100 => old: 100/66 |new: 100/66
> [100 200] arbSc 0 scMul 2 : 150/100 => old: 100/66 |new: 100/66
> [100 200] arbSc 0 scMul 3 : 150/100 => old: 100/66 |new: 100/66
> [100 200] arbSc 0 scMul 4 : 150/100 => old: 100/66 |new: 100/66
< [100 200] arbSc 1 scMul 1 : 150/100 => old: 99/66 |new: 100/66 diff 1 / 0
< [100 200] arbSc 1 scMul 2 : 150/100 => old: 99/66 |new: 100/66 diff 1 / 0
< [100 200] arbSc 1 scMul 3 : 150/100 => old: 99/66 |new: 100/66 diff 1 / 0
< [100 200] arbSc 1 scMul 4 : 150/100 => old: 99/66 |new: 100/66 diff 1 / 0
< [150 100] arbSc 0 scMul 1 : 22/44 => old: 50/100 |new: 22/44 diff -28 / -56
< [150 100] arbSc 0 scMul 2 : 22/44 => old: 50/100 |new: 22/44 diff -28 / -56
< [150 100] arbSc 0 scMul 3 : 22/44 => old: 50/100 |new: 22/44 diff -28 / -56
< [150 100] arbSc 0 scMul 4 : 22/44 => old: 50/100 |new: 22/44 diff -28 / -56
> [150 100] arbSc 1 scMul 1 : 22/44 => old: 22/44 |new: 22/44
> [150 100] arbSc 1 scMul 2 : 22/44 => old: 44/88 |new: 44/88
< [150 100] arbSc 1 scMul 3 : 22/44 => old: 49/99 |new: 50/100 diff 1 / 1
< [150 100] arbSc 1 scMul 4 : 22/44 => old: 49/99 |new: 50/100 diff 1 / 1
< [150 100] arbSc 0 scMul 1 : 44/22 => old: 150/75 |new: 44/22 diff -106 / -53
< [150 100] arbSc 0 scMul 2 : 44/22 => old: 88000/44000 |new: 88/44 diff -87912 / -43956
< [150 100] arbSc 0 scMul 3 : 44/22 => old: 88000/44000 |new: 88/44 diff -87912 / -43956
< [150 100] arbSc 0 scMul 4 : 44/22 => old: 88000/44000 |new: 88/44 diff -87912 / -43956
> [150 100] arbSc 1 scMul 1 : 44/22 => old: 44/22 |new: 44/22
> [150 100] arbSc 1 scMul 2 : 44/22 => old: 88/44 |new: 88/44
> [150 100] arbSc 1 scMul 3 : 44/22 => old: 132/66 |new: 132/66
< [150 100] arbSc 1 scMul 4 : 44/22 => old: 149/74 |new: 150/75 diff 1 / 1
> [150 100] arbSc 0 scMul 1 : 100/200 => old: 50/100 |new: 50/100
> [150 100] arbSc 0 scMul 2 : 100/200 => old: 50/100 |new: 50/100
> [150 100] arbSc 0 scMul 3 : 100/200 => old: 50/100 |new: 50/100
> [150 100] arbSc 0 scMul 4 : 100/200 => old: 50/100 |new: 50/100
> [150 100] arbSc 1 scMul 1 : 100/200 => old: 50/100 |new: 50/100
> [150 100] arbSc 1 scMul 2 : 100/200 => old: 50/100 |new: 50/100
> [150 100] arbSc 1 scMul 3 : 100/200 => old: 50/100 |new: 50/100
> [150 100] arbSc 1 scMul 4 : 100/200 => old: 50/100 |new: 50/100
> [150 100] arbSc 0 scMul 1 : 150/100 => old: 150/100 |new: 150/100
> [150 100] arbSc 0 scMul 2 : 150/100 => old: 150/100 |new: 150/100
> [150 100] arbSc 0 scMul 3 : 150/100 => old: 150/100 |new: 150/100
> [150 100] arbSc 0 scMul 4 : 150/100 => old: 150/100 |new: 150/100
> [150 100] arbSc 1 scMul 1 : 150/100 => old: 150/100 |new: 150/100
> [150 100] arbSc 1 scMul 2 : 150/100 => old: 150/100 |new: 150/100
> [150 100] arbSc 1 scMul 3 : 150/100 => old: 150/100 |new: 150/100
> [150 100] arbSc 1 scMul 4 : 150/100 => old: 150/100 |new: 150/100

NiLuJe · 2021-02-15T00:13:28Z

The good news is I get similar results w/ my code above, after a simple fix to account for truncation of the scaled ratio:

(e.g., > -> >=) here:

if (scaled_height > height && scaled_height / height >= maxScaleMult || scaled_width > width && scaled_width / width >= maxScaleMult) {

NiLuJe · 2021-02-15T00:17:35Z

Getting rid of the magic 20 pixels fixes the arbSc 0 inconsistencies, FWIW ;).

(With results that make sense, given our understanding of the function's intent).

NiLuJe · 2021-02-15T00:32:06Z

e.g., this appears to behave:

        if ( !arbitraryImageScaling ) {
            // Integer scaling. Try some integer scales first.
            if ( maxScaleMult >= 3 && height*3 < maxh && width*3 < maxw ) {
                width = width * 3;
                height = height * 3;
                return;
            }
            if ( maxScaleMult >= 2 && height * 2 < maxh && width * 2 < maxw ) {
                width = width * 2;
                height = height * 2;
                return;
            }
            if ( height <= maxh && width <= maxw ) {
                // Fits without scaling (one).
                return;
            }
            // Fall through to arbitrary scale down.
        }

        // Make sure we never blow past maxScaleMult while still fitting inside maxw/maxh
        int bbox_width = width * maxScaleMult > maxw ? maxw : width * maxScaleMult;
        int bbox_height = height * maxScaleMult > maxh ? maxh : height * maxScaleMult;

        // We want to fit *inside* the bbox, so this is a good start, as we'll ultimately keep that value for one side.
        int scaled_width = bbox_width;
        int scaled_height = bbox_height;

        // And now see whether we need to compute width or height to honor the AR.
        int rescaled_width = bbox_height * width / height;
        if (rescaled_width <= bbox_width) {
                scaled_width = rescaled_width;
        } else {
                scaled_height = bbox_width * height / width;
        }

        // We're done, update out pointers
        width = scaled_width;
        height = scaled_height;

poire-z · 2021-02-15T00:40:13Z

Getting rid of the magic 20 pixels fixes the arbSc 0 inconsistencies, FWIW ;).

But the old one had these magic 20 (although using it a bit differently), and did not stop scaling. Why?

A little comment (as you've already put a few, why not one more, to help those who suck at arithmetics :) explaining me why I don't have to wonder why you just compute int rescaled_width and never a rescaled_height would be nice :)

NiLuJe · 2021-02-15T00:55:17Z

Because the * 1000 shenanigans relegated those 20 to noise, I guess?

That's the first comment, we only need to compute a single side since we want to fit inside a bbox and keep AR: one side will be the bbox value itself. Hence, start by initializing the final values to the bbox for both sides, since we'll ultimately keep one of those, and then compute one of those sides (we could invert the logic depending on whether we mostly scale to portrait or landscape bboxes) to see if that's the one we actually want to keep, depending on whether it fits or not.

EDIT: Tweaked comments above.

NiLuJe · 2021-02-15T00:59:44Z

e.g., if you prefer, this block can read:

        int scaled_width;
        int scaled_height;
        // And now see whether we need to compute width or height to honor the AR.
        int rescaled_width = bbox_height * width / height;
        if (rescaled_width <= bbox_width) {
                scaled_width = rescaled_width;
                scaled_height = bbox_height;
        } else {
                scaled_width = bbox_width;
                scaled_height = bbox_width * height / width;
        }

poire-z · 2021-02-15T01:07:48Z

OK thanks. Yes, that reads better.
Feel free to do a PR with your code (I'm still not super at ease with all that, but the more comments about what we do, the better if we later have to look at that) if the results match and make sense with our understanding (I'll double check :)

NiLuJe · 2021-02-15T01:12:41Z

Well, it's still more than half @ourairquality's ;p.

I can take a look at it later (probably not tomorrow, though), but you have my blessing to fold it with whatever else you may have in the pipes in the meantime ;).

NiLuJe · 2021-02-15T19:26:08Z

Simplified the integer scaling codepath, and made it actually honor maxScaleMult, instead of arbitrarily stopping at 3?

    if ( !arbitraryImageScaling ) {
        // Integer scaling, constrained to maxScaleMult
        for ( int i = maxScaleMult; i > 0; i-- ) {
            // Use the largest integer multiplier that fits
            int scaled_width = width * i;
            int scaled_height = height * i;
            if ( scaled_width <= maxw && scaled_height <= maxh ) {
                width = scaled_width;
                height = scaled_height;
                return;
            }
        }

        // Fall through to arbitrary scaling
    }

    // Make sure we never blow past maxScaleMult while still fitting inside maxw/maxh
    int bbox_width = width * maxScaleMult > maxw ? maxw : width * maxScaleMult;
    int bbox_height = height * maxScaleMult > maxh ? maxh : height * maxScaleMult;

    int scaled_width;
    int scaled_height;
    // And now see whether we need to compute width or height to honor the AR.
    // c.f., QSize::scaled @ https://github.com/qt/qtbase/blob/dev/src/corelib/tools/qsize.cpp for Qt::KeepAspectRatio
    int rescaled_width = bbox_height * width / height;
    if ( rescaled_width <= bbox_width ) {
        scaled_width = rescaled_width;
        scaled_height = bbox_height;
    } else {
        scaled_width = bbox_width;
        scaled_height = bbox_width * height / width;
    }

    // We're done, update out pointers
    width = scaled_width;
    height = scaled_height;

I'll give it a proper look tomorrow, sleep deprivation and maths don't mix well ^^.

NiLuJe · 2021-02-15T19:29:13Z

Note to self: simplify the guards to be <= 0 for everybody in every codepath, because 0/negative dims make no sense, regardless of crashing considerations.

poire-z · 2021-02-15T19:38:11Z

I'll definititely let this one to you as you seem to have fun :)
(I was going with a commented version of the "new" code, just because it's shorter and 2 tests:

        // See if using maxScaleMult (possibly 1) we would overflow maxw/maxh
        if (width * maxh > height * maxw) {
            // Keeping aspect ratio, width would overflow first: so check width
            if (width * maxScaleMult > maxw) {
                // width overflows: cap width, compute height keeping A/R
                height = height * maxw / width;
                width = maxw;
                return;
            }
        }
        else {
            // Keeping aspect ratio, height would overflow first: so check height
            if (height * maxScaleMult > maxh) {
                // height overflows: cap height, compute width keeping A/R
                width = width * maxh / height;
                height = maxh;
                return;
            }
        }
        // No overflow: just scale
        width = width * maxScaleMult;
        height = height * maxScaleMult;

)

As discussed in koreader#409 Basically: * If !arbitraryImageScaling: try integer scaling, from maxScaleMult to 1. If nothing fits the bounding box, fall back to arbitrary scaling. Get rid of the weird mutlitplier and magic scratch pixels, because when they weren't introducins egregious errors, they were actually *introducing* rounding errors! * Simplify the classic scaling code, based on Qt's algorithm. No floats or weird multiplier needed, because we *always* MUL before we DIV. And if no rounding was deemed necessary by Qt, then implicit truncation is probably good enough for us, too ;).

As discussed in #409 Basically: * If !arbitraryImageScaling: try integer scaling, from maxScaleMult to 1. If nothing fits the bounding box, fall back to arbitrary scaling. Get rid of the weird mutlitplier and magic scratch pixels, because when they weren't introducins egregious errors, they were actually *introducing* rounding errors! * Simplify the classic scaling code, based on Qt's algorithm. No floats or weird multiplier needed, because we *always* MUL before we DIV. And if no rounding was deemed necessary by Qt, then implicit truncation is probably good enough for us, too ;).

poire-z added 5 commits January 18, 2021 09:59

EPUB ncx TOC: allow items with an empty title

c701a73

CSS: parse and store min/max-width/height

9230632

Increase nb of bitmaps from 2 to 3 lUInt32 as we're going over 64 properties.

poire-z force-pushed the min_max_width_height branch from 7f70d8d to 55eb7fa Compare January 18, 2021 10:31

This was referenced Jan 18, 2021

Support for max-width #360

Closed

Merged with KOReader fork of crengine buggins/coolreader#125

Merged

NiLuJe approved these changes Jan 18, 2021

View reviewed changes

Frenzie approved these changes Jan 18, 2021

View reviewed changes

poire-z merged commit c4b6b9e into koreader:master Jan 20, 2021

poire-z deleted the min_max_width_height branch January 20, 2021 12:27

poire-z mentioned this pull request Feb 3, 2021

Inadequate scaling of images in fb2 and epub files koreader/koreader#3416

Closed

This comment has been minimized.

Sign in to view

NiLuJe mentioned this pull request Feb 16, 2021

resizeImage logic fixes #421

Merged

virxkane mentioned this pull request Jun 11, 2021

Updates from koreader/crengine buggins/coolreader#299

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSS: add support for min-width, max-width, min-height, max-height #409

CSS: add support for min-width, max-width, min-height, max-height #409

poire-z commented Jan 18, 2021 •

edited by Frenzie

Loading

NiLuJe left a comment

Frenzie left a comment

poire-z commented Feb 14, 2021

poire-z commented Feb 14, 2021

NiLuJe commented Feb 14, 2021

poire-z commented Feb 14, 2021

NiLuJe commented Feb 14, 2021

NiLuJe commented Feb 14, 2021 •

edited

Loading

NiLuJe commented Feb 14, 2021 •

edited

Loading

poire-z commented Feb 14, 2021

NiLuJe commented Feb 14, 2021 •

edited

Loading

NiLuJe commented Feb 14, 2021

poire-z commented Feb 14, 2021

NiLuJe commented Feb 14, 2021 •

edited

Loading

poire-z commented Feb 14, 2021

NiLuJe commented Feb 15, 2021

NiLuJe commented Feb 15, 2021 •

edited

Loading

This comment has been minimized.

NiLuJe commented Feb 15, 2021 •

edited

Loading

poire-z commented Feb 15, 2021

NiLuJe commented Feb 15, 2021 •

edited

Loading

NiLuJe commented Feb 15, 2021

poire-z commented Feb 15, 2021

NiLuJe commented Feb 15, 2021

NiLuJe commented Feb 15, 2021 •

edited

Loading

NiLuJe commented Feb 15, 2021

poire-z commented Feb 15, 2021

CSS: add support for min-width, max-width, min-height, max-height #409

CSS: add support for min-width, max-width, min-height, max-height #409

Conversation

poire-z commented Jan 18, 2021 • edited by Frenzie Loading

NiLuJe left a comment

Choose a reason for hiding this comment

Frenzie left a comment

Choose a reason for hiding this comment

poire-z commented Feb 14, 2021

poire-z commented Feb 14, 2021

NiLuJe commented Feb 14, 2021

poire-z commented Feb 14, 2021

NiLuJe commented Feb 14, 2021

NiLuJe commented Feb 14, 2021 • edited Loading

NiLuJe commented Feb 14, 2021 • edited Loading

poire-z commented Feb 14, 2021

NiLuJe commented Feb 14, 2021 • edited Loading

NiLuJe commented Feb 14, 2021

poire-z commented Feb 14, 2021

NiLuJe commented Feb 14, 2021 • edited Loading

poire-z commented Feb 14, 2021

NiLuJe commented Feb 15, 2021

NiLuJe commented Feb 15, 2021 • edited Loading

This comment has been minimized.

NiLuJe commented Feb 15, 2021 • edited Loading

poire-z commented Feb 15, 2021

NiLuJe commented Feb 15, 2021 • edited Loading

NiLuJe commented Feb 15, 2021

poire-z commented Feb 15, 2021

NiLuJe commented Feb 15, 2021

NiLuJe commented Feb 15, 2021 • edited Loading

NiLuJe commented Feb 15, 2021

poire-z commented Feb 15, 2021

poire-z commented Jan 18, 2021 •

edited by Frenzie

Loading

NiLuJe commented Feb 14, 2021 •

edited

Loading

NiLuJe commented Feb 14, 2021 •

edited

Loading

NiLuJe commented Feb 14, 2021 •

edited

Loading

NiLuJe commented Feb 14, 2021 •

edited

Loading

NiLuJe commented Feb 15, 2021 •

edited

Loading

NiLuJe commented Feb 15, 2021 •

edited

Loading

NiLuJe commented Feb 15, 2021 •

edited

Loading

NiLuJe commented Feb 15, 2021 •

edited

Loading