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

Image size too small or illegible #4027

Closed
srushtirk opened this issue Dec 6, 2021 · 22 comments · Fixed by #4259
Closed

Image size too small or illegible #4027

srushtirk opened this issue Dec 6, 2021 · 22 comments · Fixed by #4259
Assignees
Labels
Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@srushtirk
Copy link

Describe the bug
The image sizes for most of the card with images is too small.

To Reproduce
Steps to reproduce the behavior:

  1. Go to all lessons in the app
  2. See error in the cards with images

Expected behavior
Images should be clear and legible. Also some of them need to have a zoom in feature for better clarity and visibility.

Demonstration
Screenshots available in this drive folder: https://drive.google.com/drive/u/1/folders/1N1FClZIIwo4TT_pvuOlCW62CfoJfbW9s

Environment

  • Device/emulator being used: Pixel 6
  • Android or SDK version (e.g. Android 5 or SDK 21): Android 12
  • App version (you can get this through system app settings or via the admin controls menu in-app): 0.6-alpha-7ff1b2bbd2

Additional context
Add any other context about the problem here.

@BenHenning
Copy link
Member

BenHenning commented Dec 7, 2021

Thanks @srushtirk. This is actually an interesting problem that I'm not yet fully sure how to solve.

@seanlip this relates slightly to what Eric's working on. The problem that we're facing here is we don't know how pixel-dense the screen is going to be (many mobile screens have higher density than laptop screens despite being much smaller), which results in extra-small images. We can't arbitrarily increase the size of these images because we don't know by what factor to do that, and it will result in potentially blurred & unreadable images (in the case of PNGs).

My expectation, actually, is that creators need to upload a sufficiently large image in width/height such that they look correct on higher density Android screens (which relates to the image preview pane I suggested a number of months back), or we need to know what scaling factor to use (which probably also requires some sort of preview). Note that this will only be a problem for in-line images since the app forces thumbnails to fit predetermined density-independent boxes. Further, a stopgap here might be to enforce a min width/height, but I'm not sure what that would even be.

Do you have any thoughts? This might be an area where we need a "mini-PRD" to spec out exact needs, but I'm happy to brainstorm things directly with you in the meantime.

@BenHenning
Copy link
Member

/cc @rt4914 as well since I think you've brought this up in the past. If you have any thoughts, please feel free to add them to this discussion Rajat.

@seanlip
Copy link
Member

seanlip commented Dec 7, 2021

@BenHenning Does the concern apply to SVG images, or just PNG?

All new images are being created as SVGs, so if the concern applies only to PNGs then I think the right fix is to start an effort to transform all those PNGs to SVGs.

@seanlip seanlip assigned BenHenning and unassigned seanlip Dec 7, 2021
@BenHenning
Copy link
Member

BenHenning commented Dec 7, 2021 via email

@seanlip
Copy link
Member

seanlip commented Dec 7, 2021

OK, sounds good. I'll leave this assigned to you for now, then; please let me know once there's a specific set of cases to show in the creator preview, and we can build that in.

( /cc @EricZLou for info )

@BenHenning BenHenning added this to the Alpha MR4 milestone Dec 8, 2021
@BenHenning
Copy link
Member

To clarify: I think the fix here is just figuring out a way to scale existing images based on dp rather than pixels, but it's not yet clear if we need to any additional transformations, or whether this will even work.

@BenHenning
Copy link
Member

As discussed, assigning this to you @rishidyno.

@BenHenning BenHenning assigned rishidyno and unassigned BenHenning Jan 13, 2022
@BenHenning BenHenning assigned BenHenning and unassigned rishidyno Mar 9, 2022
@BenHenning
Copy link
Member

Taking this due to the time sensitivity.

@srushtirk FYI that I mainly plan to focus on image size & being cut-off, and only short-term fixes (if possible). Other issues such as long-term zooming or contrast problems should be filed as separate issues since otherwise it's difficult to understand when this problem is actually fixed, and which parts are essential for MR4.

@BenHenning
Copy link
Member

From investigations:

  1. It seems using dp instead of sp for block drawables seems to work really well for SVGs & PNGs alike, though there's a separate SVG problem (see (3))
  2. The cutoff is happening from the image alignment logic not properly accounting for the TextView's own padding. Adjusting for this leads to what appears to be perfectly centered & sized image. There may be other cases that I'm missing, but I think this will cover at least the content text view (which covers most cases)
  3. SVGs right now can be passed in with an uncomputed instrinsic size which leads to the image completely being broken (see the first state of 'Evaluating Expressions in Expressions and Equations'). I think this happens when the SVG doesn't have defined dimensions that AndroidSVG can correctly extract (though from inspection the viewBox is still defined correctly)

Picture for (1):
image

Picture for (2):
image

Picture for (3):
image

I think we can fix (3) by defaulting to the dimensions in the image filename if we can't properly extract them from the SVG itself. Trying that out.

@BenHenning
Copy link
Member

Hmm, it seems like the filenames can actually be wrong. See:

image

The filename indicates this image's size is 200x150 (img_20210918_224300_myiu2mrtj5_height_150_width_200.svg) but per the viewbox it's actually 248.14x186.01. @seanlip is this expected (specifically that the sizes in image filenames can be wrong for SVGs)?

@seanlip
Copy link
Member

seanlip commented Mar 10, 2022

On the website, that SVG is displayed in a 200 x 150 container, so I think the sizing for it is just incorrect. Given that, I think it should still be OK to use the dimensions in the filename...

@seanlip
Copy link
Member

seanlip commented Mar 10, 2022

As an update -- I've updated the relevant image in the Oppia website.

@BenHenning
Copy link
Member

Thanks @seanlip. It actually isn't okay for us to use those dimensions since these are the actual width/height of the picture to render (since we pre-render the SVG to a bitmap, we need to know the exact size). Since the dimensions are wrong, the image gets cut off as indicated in the screenshot above. I suppose we could use the file dimensions as a signal, but it seems that we'll also need to use the view box dimensions as a signal, too.

@seanlip
Copy link
Member

seanlip commented Mar 10, 2022

Hm -- the size of the SVG on web is defined only by the filename dimensions, and not by the viewbox dimensions at all, AFAIK.

@seanlip
Copy link
Member

seanlip commented Mar 10, 2022

Also, just as a data point -- on Web, when I change the height/width of the image tag to be smaller than the viewbox dimensions, the image just rescales accordingly (and isn't cut off). From an aspect ratio perspective it ends up being bounded in a rectangle whose height/width is based on the most constrained dimension (height/width) of the img tag.

@BenHenning
Copy link
Member

@seanlip I think you don't run into this problem on web since I'm assuming you're not actually responsible for setting the dimensions of the SVG to render, only the dimensions you want to scale it to. The browser presumably parses the SVG to determine what size it expects to be rendered at.

@seanlip
Copy link
Member

seanlip commented Mar 10, 2022

Yup I think that's right. See comment above.

@BenHenning
Copy link
Member

So the screenshot above indicates we can use the view box as a source of information if the document width isn't available. However, this actually breaks thumbnails that seem to have some sort of rendering inconsistency:

image

The local size of the SVG when rendered and the size of the viewbox match, but when we actually render it with AndroidSVG it only renders part of it. I'm guessing it's doing some sort of relative rendering, but I'll need to dig a bit more.

@BenHenning
Copy link
Member

Unfortunately, I lost my draft from last week and didn't post it. The fix here was updating AndroidSVG to use the viewbox dimensions for the rendering canvas if no dimensions are provided, and when no document dimensions are defined (before, it would default to 512x512 which is why the image ended up being cropped). The result (note that this includes some additional resizing logic to make the image higher-resolution, but I'll be postponing finishing that part since I'm having difficulty getting SVGs to not be fuzzy unlike bitmaps, and there are some performance issues cropping up):

image

@BenHenning
Copy link
Member

Scaling and using an adaptive technique for finding image sizes seems to work well:

image

We now source the size from several different locations to try and determine the correct size.

BenHenning added a commit that referenced this issue Mar 19, 2022
No tests or updated documentation.
BenHenning added a commit that referenced this issue May 6, 2022
…s bugs found while testing alpha MR4 (#4259)

* Fix for #4040.

Doesn't include test changes yet.

* Fixes for #4027.

No tests or updated documentation.

* Revert development-only caching flags.

* Lint fixes.

* Post-merge fix + 2 other fixes.

The first fix addresses #4076 in full.

The second fix addresses what seems to be a long-standing issue with
image region selection to ensure that the region views are always
clickable when the interaction loads.

* Add follow-up fixes & tests.

* Fix new test on Gradle builds.

This test corresponds to Bazel-locked functionality, so it can't pass on
Gradle in the same way.

* Fix #4319 and #4329.

* Post-merge lint fixes.

* Lint fixes, + doc & TODO udpate.
@KolliAnitha
Copy link

Verified in 0.7-alpha-6c08d882d3. No errors are seen in the images.

@BenHenning
Copy link
Member

For searching context, this issue was found in 0.6-alpha (MR3).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-ibt Temporary label for Ben to keep track of issues he's triaged.
5 participants