fix: harden GIF-to-MP4 converter after code review

Address issues found in Kotlin code review:

HIGH
- Rethrow CancellationException in convert() to preserve structured
  concurrency; prior broad catch silently swallowed cancellation.
- Cap drainEncoder's post-EOS loop at 500 iterations to prevent an
  unresponsive hardware encoder from hanging the IO thread forever.

MEDIUM
- Read GIF with a 20 MB size cap via bounded buffer to avoid OOM on
  malformed or adversarial inputs.
- Check glCompileShader / glLinkProgram status and throw with
  glGetShaderInfoLog / glGetProgramInfoLog on failure, instead of
  silently producing blank frames on driver errors.
- Verify numConfigs[0] > 0 after eglChooseConfig and use requireNotNull
  on configs[0] with a clear message (avoids NPE from !!).
- Fix Image Descriptor (0x2C) parsing: precheck pos + 10 <= bytes.size
  and read the packed byte at the correct offset; bounds-check after
  Local Color Table skip.
- Use uris.hasVideo() instead of uris.first().media.isVideo() for the
  privacy-toggle visibility so mixed-media selections (image + video)
  hide the toggle correctly.

LOW
- skipSubBlocks now clamps pos with minOf(pos + blockSize, bytes.size)
  to preserve the invariant that pos is always a valid index.
- Document the function-level @Suppress("deprecation") on
  convertInternal (android.graphics.Movie has no modern replacement).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
davotoula
2026-04-09 09:00:01 +02:00
parent 613237bdb9
commit e4c12ff5ad
2 changed files with 117 additions and 31 deletions
@@ -41,6 +41,7 @@ import android.opengl.GLUtils
import android.view.Surface import android.view.Surface
import androidx.core.net.toUri import androidx.core.net.toUri
import com.vitorpamplona.quartz.utils.Log import com.vitorpamplona.quartz.utils.Log
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext import kotlinx.coroutines.withContext
import java.io.File import java.io.File
@@ -57,15 +58,29 @@ object GifToMp4Converter {
private const val DEFAULT_FRAME_DELAY_MS = 100 private const val DEFAULT_FRAME_DELAY_MS = 100
private const val US_PER_MS = 1000L private const val US_PER_MS = 1000L
private const val NS_PER_US = 1000L private const val NS_PER_US = 1000L
private const val MAX_GIF_SIZE_BYTES = 20 * 1024 * 1024
private const val DRAIN_EOS_MAX_ITERATIONS = 500
// Fullscreen quad: 4 vertices x (2 position + 2 texcoord) floats // Fullscreen quad: 4 vertices x (2 position + 2 texcoord) floats
// Texcoord Y is flipped because bitmap origin is top-left, GL is bottom-left // Texcoord Y is flipped because bitmap origin is top-left, GL is bottom-left
private val QUAD_COORDS = private val QUAD_COORDS =
floatArrayOf( floatArrayOf(
-1f, -1f, 0f, 1f, -1f,
1f, -1f, 1f, 1f, -1f,
-1f, 1f, 0f, 0f, 0f,
1f, 1f, 1f, 0f, 1f,
1f,
-1f,
1f,
1f,
-1f,
1f,
0f,
0f,
1f,
1f,
1f,
0f,
) )
private const val VERTEX_SHADER = private const val VERTEX_SHADER =
@@ -92,23 +107,37 @@ object GifToMp4Converter {
withContext(Dispatchers.IO) { withContext(Dispatchers.IO) {
try { try {
convertInternal(uri, context) convertInternal(uri, context)
} catch (e: CancellationException) {
throw e
} catch (e: Exception) { } catch (e: Exception) {
Log.e(LOG_TAG, "GIF to MP4 conversion failed", e) Log.e(LOG_TAG, "GIF to MP4 conversion failed", e)
null null
} }
} }
@Suppress("deprecation") @Suppress("deprecation") // android.graphics.Movie is deprecated but still the simplest GIF decoder available
private fun convertInternal( private fun convertInternal(
uri: Uri, uri: Uri,
context: Context, context: Context,
): MediaCompressorResult? { ): MediaCompressorResult? {
val gifBytes = val gifBytes =
context.contentResolver.openInputStream(uri)?.use { it.readBytes() } context.contentResolver.openInputStream(uri)?.use { input ->
?: run { val buffer = ByteArray(MAX_GIF_SIZE_BYTES + 1)
Log.w(LOG_TAG) { "Failed to read GIF bytes" } var total = 0
while (total <= MAX_GIF_SIZE_BYTES) {
val read = input.read(buffer, total, buffer.size - total)
if (read == -1) break
total += read
}
if (total > MAX_GIF_SIZE_BYTES) {
Log.w(LOG_TAG) { "GIF exceeds max size of $MAX_GIF_SIZE_BYTES bytes" }
return null return null
} }
buffer.copyOf(total)
} ?: run {
Log.w(LOG_TAG) { "Failed to read GIF bytes" }
return null
}
val movie = val movie =
Movie.decodeByteArray(gifBytes, 0, gifBytes.size) Movie.decodeByteArray(gifBytes, 0, gifBytes.size)
@@ -228,18 +257,38 @@ object GifToMp4Converter {
if (outputFile.exists()) outputFile.delete() if (outputFile.exists()) outputFile.delete()
return null return null
} finally { } finally {
try { egl?.release() } catch (_: Exception) { } try {
try { codec?.stop() } catch (_: Exception) { } egl?.release()
try { codec?.release() } catch (_: Exception) { } } catch (_: Exception) {
try { codecSurface?.release() } catch (_: Exception) { } }
try { muxer?.stop() } catch (_: Exception) { } try {
try { muxer?.release() } catch (_: Exception) { } codec?.stop()
} catch (_: Exception) {
}
try {
codec?.release()
} catch (_: Exception) {
}
try {
codecSurface?.release()
} catch (_: Exception) {
}
try {
muxer?.stop()
} catch (_: Exception) {
}
try {
muxer?.release()
} catch (_: Exception) {
}
} }
} }
// region EGL // region EGL
private class EglHelper(surface: Surface) { private class EglHelper(
surface: Surface,
) {
val display: EGLDisplay val display: EGLDisplay
val context: EGLContext val context: EGLContext
val eglSurface: EGLSurface val eglSurface: EGLSurface
@@ -253,12 +302,18 @@ object GifToMp4Converter {
val configAttribs = val configAttribs =
intArrayOf( intArrayOf(
EGL14.EGL_RED_SIZE, 8, EGL14.EGL_RED_SIZE,
EGL14.EGL_GREEN_SIZE, 8, 8,
EGL14.EGL_BLUE_SIZE, 8, EGL14.EGL_GREEN_SIZE,
EGL14.EGL_ALPHA_SIZE, 8, 8,
EGL14.EGL_RENDERABLE_TYPE, EGL14.EGL_OPENGL_ES2_BIT, EGL14.EGL_BLUE_SIZE,
EGL14.EGL_SURFACE_TYPE, EGL14.EGL_WINDOW_BIT, 8,
EGL14.EGL_ALPHA_SIZE,
8,
EGL14.EGL_RENDERABLE_TYPE,
EGL14.EGL_OPENGL_ES2_BIT,
EGL14.EGL_SURFACE_TYPE,
EGL14.EGL_WINDOW_BIT,
EGL14.EGL_NONE, EGL14.EGL_NONE,
) )
val configs = arrayOfNulls<EGLConfig>(1) val configs = arrayOfNulls<EGLConfig>(1)
@@ -266,13 +321,15 @@ object GifToMp4Converter {
check(EGL14.eglChooseConfig(display, configAttribs, 0, configs, 0, 1, numConfigs, 0)) { check(EGL14.eglChooseConfig(display, configAttribs, 0, configs, 0, 1, numConfigs, 0)) {
"eglChooseConfig failed" "eglChooseConfig failed"
} }
check(numConfigs[0] > 0) { "eglChooseConfig returned no matching configs" }
val config = requireNotNull(configs[0]) { "eglChooseConfig returned null config" }
val contextAttribs = intArrayOf(EGL14.EGL_CONTEXT_CLIENT_VERSION, 2, EGL14.EGL_NONE) val contextAttribs = intArrayOf(EGL14.EGL_CONTEXT_CLIENT_VERSION, 2, EGL14.EGL_NONE)
context = EGL14.eglCreateContext(display, configs[0]!!, EGL14.EGL_NO_CONTEXT, contextAttribs, 0) context = EGL14.eglCreateContext(display, config, EGL14.EGL_NO_CONTEXT, contextAttribs, 0)
check(context != EGL14.EGL_NO_CONTEXT) { "eglCreateContext failed" } check(context != EGL14.EGL_NO_CONTEXT) { "eglCreateContext failed" }
val surfaceAttribs = intArrayOf(EGL14.EGL_NONE) val surfaceAttribs = intArrayOf(EGL14.EGL_NONE)
eglSurface = EGL14.eglCreateWindowSurface(display, configs[0]!!, surface, surfaceAttribs, 0) eglSurface = EGL14.eglCreateWindowSurface(display, config, surface, surfaceAttribs, 0)
check(eglSurface != EGL14.EGL_NO_SURFACE) { "eglCreateWindowSurface failed" } check(eglSurface != EGL14.EGL_NO_SURFACE) { "eglCreateWindowSurface failed" }
} }
@@ -304,9 +361,17 @@ object GifToMp4Converter {
val vs = compileShader(GLES20.GL_VERTEX_SHADER, VERTEX_SHADER) val vs = compileShader(GLES20.GL_VERTEX_SHADER, VERTEX_SHADER)
val fs = compileShader(GLES20.GL_FRAGMENT_SHADER, FRAGMENT_SHADER) val fs = compileShader(GLES20.GL_FRAGMENT_SHADER, FRAGMENT_SHADER)
val program = GLES20.glCreateProgram() val program = GLES20.glCreateProgram()
check(program != 0) { "glCreateProgram failed" }
GLES20.glAttachShader(program, vs) GLES20.glAttachShader(program, vs)
GLES20.glAttachShader(program, fs) GLES20.glAttachShader(program, fs)
GLES20.glLinkProgram(program) GLES20.glLinkProgram(program)
val linkStatus = IntArray(1)
GLES20.glGetProgramiv(program, GLES20.GL_LINK_STATUS, linkStatus, 0)
if (linkStatus[0] != GLES20.GL_TRUE) {
val info = GLES20.glGetProgramInfoLog(program)
GLES20.glDeleteProgram(program)
throw RuntimeException("glLinkProgram failed: $info")
}
GLES20.glDeleteShader(vs) GLES20.glDeleteShader(vs)
GLES20.glDeleteShader(fs) GLES20.glDeleteShader(fs)
return program return program
@@ -317,8 +382,16 @@ object GifToMp4Converter {
source: String, source: String,
): Int { ): Int {
val shader = GLES20.glCreateShader(type) val shader = GLES20.glCreateShader(type)
check(shader != 0) { "glCreateShader failed for type $type" }
GLES20.glShaderSource(shader, source) GLES20.glShaderSource(shader, source)
GLES20.glCompileShader(shader) GLES20.glCompileShader(shader)
val compileStatus = IntArray(1)
GLES20.glGetShaderiv(shader, GLES20.GL_COMPILE_STATUS, compileStatus, 0)
if (compileStatus[0] != GLES20.GL_TRUE) {
val info = GLES20.glGetShaderInfoLog(shader)
GLES20.glDeleteShader(shader)
throw RuntimeException("glCompileShader failed for type $type: $info")
}
return shader return shader
} }
@@ -429,18 +502,28 @@ object GifToMp4Converter {
pos = skipSubBlocks(bytes, pos) pos = skipSubBlocks(bytes, pos)
} }
} }
0x2C -> { 0x2C -> {
// Image Descriptor: 1 (separator, current pos) + 8 (L/T/W/H) + 1 (packed) = 10 bytes
if (pos + 10 > bytes.size) break
val imgPacked = bytes[pos + 9].toInt() and 0xFF
pos += 10 pos += 10
if (pos > bytes.size) break
val imgPacked = bytes[pos - 1].toInt() and 0xFF
if (imgPacked and 0x80 != 0) { if (imgPacked and 0x80 != 0) {
pos += 3 * (1 shl ((imgPacked and 0x07) + 1)) pos += 3 * (1 shl ((imgPacked and 0x07) + 1))
if (pos >= bytes.size) break
} }
pos++ pos++
if (pos >= bytes.size) break
pos = skipSubBlocks(bytes, pos) pos = skipSubBlocks(bytes, pos)
} }
0x3B -> break
else -> pos++ 0x3B -> {
break
}
else -> {
pos++
}
} }
} }
@@ -456,7 +539,7 @@ object GifToMp4Converter {
val blockSize = bytes[pos].toInt() and 0xFF val blockSize = bytes[pos].toInt() and 0xFF
pos++ pos++
if (blockSize == 0) break if (blockSize == 0) break
pos += blockSize pos = minOf(pos + blockSize, bytes.size)
} }
return pos return pos
} }
@@ -475,6 +558,7 @@ object GifToMp4Converter {
): Pair<Int, Boolean> { ): Pair<Int, Boolean> {
var currentTrackIndex = trackIndex var currentTrackIndex = trackIndex
var currentMuxerStarted = muxerStarted var currentMuxerStarted = muxerStarted
var eosDrainIterations = 0
while (true) { while (true) {
val outputIndex = codec.dequeueOutputBuffer(bufferInfo, TIMEOUT_US) val outputIndex = codec.dequeueOutputBuffer(bufferInfo, TIMEOUT_US)
@@ -482,6 +566,9 @@ object GifToMp4Converter {
when { when {
outputIndex == MediaCodec.INFO_TRY_AGAIN_LATER -> { outputIndex == MediaCodec.INFO_TRY_AGAIN_LATER -> {
if (!endOfStream) return Pair(currentTrackIndex, currentMuxerStarted) if (!endOfStream) return Pair(currentTrackIndex, currentMuxerStarted)
if (++eosDrainIterations >= DRAIN_EOS_MAX_ITERATIONS) {
throw RuntimeException("Encoder failed to drain after EOS within ${DRAIN_EOS_MAX_ITERATIONS} iterations")
}
} }
outputIndex == MediaCodec.INFO_OUTPUT_FORMAT_CHANGED -> { outputIndex == MediaCodec.INFO_OUTPUT_FORMAT_CHANGED -> {
@@ -252,9 +252,8 @@ fun ImageVideoDescription(
) )
} }
// Hide privacy toggle when video compression is selected (compression already strips metadata) // Hide privacy toggle when any selected video will be compressed (compression already strips metadata)
val isVideoWithCompression = val isVideoWithCompression = uris.hasVideo() && mediaQualitySlider != 3
uris.first().media.isVideo() == true && mediaQualitySlider != 3
if (!isVideoWithCompression) { if (!isVideoWithCompression) {
SettingSwitchItem( SettingSwitchItem(