Skip to content

Modernise GifTranscoder JNI files#192

Open
mikomikotaishi wants to merge 2 commits into
GrapheneOS:mainfrom
mikomikotaishi:main
Open

Modernise GifTranscoder JNI files#192
mikomikotaishi wants to merge 2 commits into
GrapheneOS:mainfrom
mikomikotaishi:main

Conversation

@mikomikotaishi

Copy link
Copy Markdown

This PR addresses the following in the native JNI parts of GifTranscoder:

  • Set an explicit C++ version (C++17), which is the NDK default
  • Replace macros with constexpr functions and constants where possible
  • Add [[nodiscard]] attributes, to explicitly highlight when a function call's return goes unused
  • Add noexcept specifiers
  • Replace C-style castes (T) with static_cast<T> and reinterpret_cast<T>
  • Replace NULL with nullptr
  • Replace a typedef with a using alias
  • Replace bodyless constructor/destructor with = default
  • Replace int[4] with std::array<int, 4>
  • Replace the C ::timespec and ::clock_gettime() with std::chrono::steady_clock
  • Explicitly disable copy constructor and copy assignment operators on GifFilesCloser

@mikomikotaishi

Copy link
Copy Markdown
Author

@m4pl @RankoR @thestinger I'd like to request a PR review (I do not have access to assign reviewers)

@RankoR
RankoR requested review from RankoR and m4pl July 10, 2026 17:19

@RankoR RankoR left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.

It's good in general, but could you please do these adjustments?

  1. Update the PR description and commit message to explicitly mention the x86_64 signed-char undefined behavior and color-corruption fix
  2. Add a regression test using color channels above 0x7f. An end-to-end test that transcodes the small 4×2 fixture and checks the resulting two pixels would catch the old implementation
  3. Keep the functional fix in a separate commit from the mechanical modernization

@mikomikotaishi

Copy link
Copy Markdown
Author

Where would you like the test to go?

@RankoR

RankoR commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

@mikomikotaishi you can add something like app/src/androidTest/java/com/android/messaging/util/GifTranscoderTest.kt, put a GIF in app/src/androidTest/assets/gif_transcoder/high_color_channels_4x2.gif and load it from the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants