-
Notifications
You must be signed in to change notification settings - Fork 632
Fixes #6086: Add transparent image detection and math tag validation #6144
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
base: introduce-asset-download-script
Are you sure you want to change the base?
Changes from 8 commits
17910d7
8db3e0e
2609c7f
e27e6ce
2ac8193
50b8ace
7c5b892
5c2a9ab
9911111
3003dae
e9bbb0d
707c802
17b72b1
561738b
7d97b71
24f6060
f008503
b74ec9f
9ff0341
dfee115
db7d94e
1ca158e
cab5739
8759fad
825ee16
9e6794d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,20 @@ class ImageRepairer { | |
| return areImagesEqual(image1, image2) | ||
| } | ||
|
|
||
| fun hasTransparentPixels(imageData: ByteArray, extension: String): Boolean { | ||
| if (extension.equals("svg", ignoreCase = true)) return false | ||
| val image = imageData.inputStream().use { ImageIO.read(it) } | ||
| ?: error("Failed to read image data (extension: $extension, size: ${imageData.size} bytes)") | ||
| if (!image.colorModel.hasAlpha()) return false | ||
| for (y in 0 until image.height) { | ||
| for (x in 0 until image.width) { | ||
| val alpha = (image.getRGB(x, y) shr 24) and 0xff | ||
| if (alpha < FULLY_OPAQUE_ALPHA) return true | ||
| } | ||
|
Comment on lines
+58
to
+62
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion. I’m keeping this PR scoped to the reviewer-requested correctness fixes and not adding performance refactors here. I can follow up with a separate optimization PR for the transparent-pixel scan if needed. |
||
| } | ||
| return false | ||
| } | ||
|
|
||
| sealed class RepairedImage { | ||
| data class RenderedSvg( | ||
| val pngContents: List<Byte>, | ||
|
|
@@ -75,6 +89,7 @@ class ImageRepairer { | |
| private val WIDTH_REGEX by lazy { "width_(\\d+)".toRegex() } | ||
| private val HEIGHT_REGEX by lazy { "height_(\\d+)".toRegex() } | ||
| private val TRANSPARENT = Color(/* r = */ 0, /* g = */ 0, /* b = */ 0, /* a = */ 0) | ||
| private const val FULLY_OPAQUE_ALPHA = 255 | ||
|
|
||
| private const val REFERENCE_MONITOR_PPI = 81.589f | ||
| private const val RELATIVE_SIZE_ADJUSTMENT_FACTOR = 0.15f | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ package org.oppia.android.scripts.gae.compat | |
| import org.oppia.android.scripts.gae.compat.StructureCompatibilityChecker.CompatibilityFailure.AudioVoiceoverHasInvalidAudioFormat | ||
| import org.oppia.android.scripts.gae.compat.StructureCompatibilityChecker.CompatibilityFailure.HtmlInTitleOrDescription | ||
| import org.oppia.android.scripts.gae.compat.StructureCompatibilityChecker.CompatibilityFailure.HtmlUnexpectedlyInUnicodeContent | ||
| import org.oppia.android.scripts.gae.compat.StructureCompatibilityChecker.CompatibilityFailure.MathTagMissingRawLatex | ||
| import org.oppia.android.scripts.gae.compat.StructureCompatibilityChecker.CompatibilityFailure.MissingRequiredXlationLangForContentTranslation | ||
| import org.oppia.android.scripts.gae.compat.StructureCompatibilityChecker.CompatibilityFailure.MissingRequiredXlationLangForTitleOrDescFromWeb | ||
| import org.oppia.android.scripts.gae.compat.StructureCompatibilityChecker.CompatibilityFailure.StateHasInvalidInteractionId | ||
|
|
@@ -42,6 +43,7 @@ import org.oppia.android.scripts.gae.json.GaeWrittenTranslation | |
| import org.oppia.android.scripts.gae.json.GaeWrittenTranslations | ||
| import org.oppia.android.scripts.gae.json.VersionedStructure | ||
| import org.oppia.android.scripts.gae.proto.LocalizationTracker | ||
| import org.oppia.android.scripts.gae.proto.LocalizationTracker.Companion.extractMathContentsFromHtml | ||
| import org.oppia.android.scripts.gae.proto.LocalizationTracker.Companion.parseColorRgb | ||
| import org.oppia.android.scripts.gae.proto.LocalizationTracker.Companion.resolveLanguageCode | ||
| import org.oppia.android.scripts.gae.proto.LocalizationTracker.ContainerId | ||
|
|
@@ -476,6 +478,11 @@ class StructureCompatibilityChecker( | |
| val stateName: String, | ||
| override val origin: ContainerId | ||
| ) : CompatibilityFailure() | ||
|
|
||
| data class MathTagMissingRawLatex( | ||
| val contentId: String, | ||
| override val origin: ContainerId | ||
| ) : CompatibilityFailure() | ||
| } | ||
|
|
||
| private fun String.checkIsValidTopicId(origin: ContainerId): List<CompatibilityFailure> { | ||
|
|
@@ -591,7 +598,9 @@ class StructureCompatibilityChecker( | |
| } ?: TextHasInvalidTags(contentId, extraTags, origin) | ||
| listOf(failure) | ||
| } else emptyList() | ||
| return tagFailures + checkHasValidImageReferences(origin, contentId) | ||
| return tagFailures + | ||
| checkHasValidImageReferences(origin, contentId) + | ||
| checkHasValidMathTags(origin, contentId) | ||
| } | ||
|
|
||
| private fun String.checkHasValidImageReferences( | ||
|
|
@@ -608,6 +617,11 @@ class StructureCompatibilityChecker( | |
| ) | ||
| } | ||
|
|
||
| private fun String.checkHasValidMathTags( | ||
| origin: ContainerId, | ||
| contentId: String | ||
| ): List<CompatibilityFailure> = checkMathTagsForLatex(this, origin, contentId) | ||
|
|
||
| private fun Int.checkIsValidStateSchemaVersion(origin: ContainerId): List<CompatibilityFailure> { | ||
| return if (this > constraints.supportedStateSchemaVersion) { | ||
| listOf(StateSchemaVersionTooNew(schemaVersion = this, origin)) | ||
|
|
@@ -623,13 +637,27 @@ class StructureCompatibilityChecker( | |
| } else emptyList() | ||
| } | ||
|
|
||
| private companion object { | ||
| companion object { | ||
| private val HTML_PRESENCE_REGEX = "</?.+?>".toRegex() | ||
| // This regex is a simplification of the standard: https://www.w3.org/TR/xml/#NT-NameStartChar. | ||
| private val HTML_TAG_REGEX = "<\\s*([^\\s/>]+)[^>]*?>".toRegex() | ||
| private val IMAGE_TAG_REGEX = "<\\s*oppia-noninteractive-image.+?>".toRegex() | ||
| private val IMAGE_FILE_PATH_REGEX = "filepath-with-value\\s*=\\s*\"(.+?)\"".toRegex() | ||
|
|
||
| fun checkMathTagsForLatex( | ||
| html: String, | ||
| origin: ContainerId, | ||
| contentId: String | ||
| ): List<CompatibilityFailure> { | ||
| return extractMathContentsFromHtml(html).mapNotNull { mathContent -> | ||
| if (mathContent?.rawLatex.isNullOrBlank()) { | ||
| MathTagMissingRawLatex(contentId, origin) | ||
| } else { | ||
| null | ||
| } | ||
| } | ||
|
||
| } | ||
|
|
||
| private fun String.checkTitleOrDescTextForHtml( | ||
| origin: ContainerId | ||
| ): List<CompatibilityFailure> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -671,11 +671,17 @@ class LocalizationTracker private constructor( | |
| private const val CUSTOM_IMG_FILE_PATH_ATTRIBUTE = "filepath-with-value" | ||
| private const val CUSTOM_MATH_TAG = "oppia-noninteractive-math" | ||
| private const val CUSTOM_MATH_SVG_PATH_ATTRIBUTE = "math_content-with-value" | ||
| private val customMathTagContentRegex by lazy { | ||
BenHenning marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| Regex( | ||
| "<\\s*$CUSTOM_MATH_TAG[^>]*?>.*?</\\s*$CUSTOM_MATH_TAG\\s*>", | ||
nikhilkumarpanigrahi marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| setOf(RegexOption.DOT_MATCHES_ALL) | ||
| ) | ||
| } | ||
| private val customImageTagRegex by lazy { | ||
| Regex("<\\s*$CUSTOM_IMG_TAG.+?$CUSTOM_IMG_FILE_PATH_ATTRIBUTE\\s*=\\s*\"(.+?)\"") | ||
| } | ||
| private val customMathTagRegex by lazy { | ||
| Regex("<\\s*$CUSTOM_MATH_TAG.+?$CUSTOM_MATH_SVG_PATH_ATTRIBUTE\\s*=\\s*\"(.+?)\"") | ||
| Regex("$CUSTOM_MATH_SVG_PATH_ATTRIBUTE\\s*=\\s*\"(.+?)\"") | ||
| } | ||
| val VALID_LANGUAGE_TYPES = LanguageType.values().filter { it.isValid() } | ||
|
|
||
|
|
@@ -727,14 +733,32 @@ class LocalizationTracker private constructor( | |
| } | ||
|
|
||
| private fun collectMathSourcesFromHtml(html: String): Set<String> { | ||
| return customMathTagRegex.findAll(html) | ||
| .map { it.destructured } | ||
| .map { (match) -> match } | ||
| .map { it.replace("&quot;", "\"") } | ||
| .map { MathContentValue.parseFromHtmlValue(it) } | ||
| return extractMathContentsFromHtml(html) | ||
| .mapNotNull { it } | ||
| .map { it.svgFilename } | ||
| .filter { it.isNotEmpty() } // Ignore incorrect missing images. | ||
| .toSet() | ||
| } | ||
|
|
||
| fun extractMathContentsFromHtml(html: String): List<MathContentValue?> { | ||
|
||
| return extractMathTagContentValuesFromHtml(html).map { mathContentJson -> | ||
| mathContentJson?.let { | ||
| try { | ||
| MathContentValue.parseFromHtmlValue(it) | ||
| } catch (_: Exception) { | ||
| null | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun extractMathTagContentValuesFromHtml(html: String): List<String?> { | ||
| return customMathTagContentRegex.findAll(html).map { tagMatch -> | ||
| val tagContent = tagMatch.value | ||
| customMathTagRegex.find(tagContent) | ||
| ?.destructured?.let { (match) -> match } | ||
| ?.replace("&quot;", "\"") | ||
| }.toList() | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| """ | ||
| Tests corresponding to asset transformation scripts. | ||
| """ | ||
|
|
||
| load("@io_bazel_rules_kotlin//kotlin:jvm.bzl", "kt_jvm_test") | ||
|
|
||
| kt_jvm_test( | ||
| name = "ImageRepairerTest", | ||
| srcs = ["ImageRepairerTest.kt"], | ||
| deps = [ | ||
| "//scripts/src/java/org/oppia/android/scripts/assets:image_repairer", | ||
| "//third_party:com_google_truth_truth", | ||
| "//third_party:org_jetbrains_kotlin_kotlin-test-junit", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| package org.oppia.android.scripts.assets | ||
|
|
||
| import com.google.common.truth.Truth.assertThat | ||
| import org.junit.Test | ||
| import java.awt.Color | ||
| import java.awt.image.BufferedImage | ||
| import java.io.ByteArrayOutputStream | ||
| import javax.imageio.ImageIO | ||
|
|
||
| // Function name: test names are conventionally named with underscores. | ||
| @Suppress("FunctionName") | ||
| class ImageRepairerTest { | ||
| private val imageRepairer = ImageRepairer() | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_opaqueImage_returnsFalse() { | ||
| val imageData = createPngImageData(BufferedImage.TYPE_INT_ARGB) { graphics -> | ||
| graphics.color = Color(255, 0, 0, 255) | ||
| graphics.fillRect(0, 0, 2, 2) | ||
| } | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "png")).isFalse() | ||
| } | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_fullyTransparentImage_returnsTrue() { | ||
| val imageData = createPngImageData(BufferedImage.TYPE_INT_ARGB) { graphics -> | ||
| graphics.color = Color(0, 0, 0, 0) | ||
| graphics.fillRect(0, 0, 2, 2) | ||
| } | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "png")).isTrue() | ||
| } | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_partiallyTransparentPixel_returnsTrue() { | ||
| val imageData = createPngImageData(BufferedImage.TYPE_INT_ARGB) { graphics -> | ||
| graphics.color = Color(255, 0, 0, 128) | ||
| graphics.fillRect(0, 0, 2, 2) | ||
| } | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "png")).isTrue() | ||
| } | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_imageWithoutAlphaChannel_returnsFalse() { | ||
| val imageData = createPngImageData(BufferedImage.TYPE_INT_RGB) { graphics -> | ||
| graphics.color = Color.RED | ||
| graphics.fillRect(0, 0, 2, 2) | ||
| } | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "png")).isFalse() | ||
| } | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_svgExtension_returnsFalse() { | ||
| val imageData = "<svg></svg>".toByteArray() | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "svg")).isFalse() | ||
| } | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_svgExtensionUpperCase_returnsFalse() { | ||
| val imageData = "<svg></svg>".toByteArray() | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "SVG")).isFalse() | ||
| } | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_mixedOpaqueAndTransparent_returnsTrue() { | ||
| val image = BufferedImage(2, 2, BufferedImage.TYPE_INT_ARGB) | ||
| image.setRGB(0, 0, Color(255, 0, 0, 255).rgb) | ||
| image.setRGB(1, 0, Color(0, 255, 0, 255).rgb) | ||
| image.setRGB(0, 1, Color(0, 0, 255, 100).rgb) | ||
| image.setRGB(1, 1, Color(255, 255, 0, 255).rgb) | ||
| val imageData = ByteArrayOutputStream().also { | ||
| ImageIO.write(image, "png", it) | ||
| }.toByteArray() | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "png")).isTrue() | ||
| } | ||
|
|
||
| private fun createPngImageData( | ||
| imageType: Int, | ||
| draw: (java.awt.Graphics2D) -> Unit | ||
| ): ByteArray { | ||
| val image = BufferedImage(2, 2, imageType) | ||
| val graphics = image.createGraphics() | ||
| draw(graphics) | ||
| graphics.dispose() | ||
| return ByteArrayOutputStream().also { | ||
| ImageIO.write(image, "png", it) | ||
| }.toByteArray() | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| """ | ||
| Tests corresponding to structure compatibility checking scripts. | ||
| """ | ||
|
|
||
| load("@io_bazel_rules_kotlin//kotlin:jvm.bzl", "kt_jvm_test") | ||
|
|
||
| kt_jvm_test( | ||
| name = "StructureCompatibilityCheckerTest", | ||
| srcs = ["StructureCompatibilityCheckerTest.kt"], | ||
| deps = [ | ||
| "//scripts/src/java/org/oppia/android/scripts/gae/compat", | ||
| "//scripts/src/java/org/oppia/android/scripts/gae/proto:localization_tracker", | ||
| "//third_party:com_google_truth_truth", | ||
| "//third_party:org_jetbrains_kotlin_kotlin-test-junit", | ||
| ], | ||
| ) |
Uh oh!
There was an error while loading. Please reload this page.