feat: add CGEHistogramFilter — luminance histogram overlay via GLES 3.1 compute shaders#567
feat: add CGEHistogramFilter — luminance histogram overlay via GLES 3.1 compute shaders#567
Conversation
Implement a luminance histogram overlay filter using three GPU-side
compute-shader passes:
Pass 1 (clear) - zero the 256x1 r32ui atomic counter texture
Pass 2 (count) - imageAtomicAdd per pixel into the counter texture
Pass 3 (draw) - render a normalised bar chart into a 256x256
rgba8 display texture
The display texture is blended onto the frame at the configured
position/size with 80% alpha, identical to CGEWaveformFilter.
Filter rule syntax:
@Style hist <left> <top> <width> <height>
e.g. @Style hist 0.01 0.01 0.4 0.4
Changes:
- Add cgeHistogramFilter.h / .cpp (new filter)
- Register createHistogramFilter() in cgeAdvancedEffects.h/.cpp
- Add 'hist' style parser in cgeDataParsingEngine.cpp
- Add two demo entries in MainActivity.java EFFECT_CONFIGS
Requires OpenGL ES 3.1+ (compute shaders + imageAtomicAdd).
WalkthroughA new GPU-based histogram filter feature is introduced using GLES 3.1 compute shaders. Changes span configuration integration, factory pattern setup, data parsing for the new "hist" style parameter, and complete histogram filter implementation with texture-based computation and overlay rendering. Changes
Sequence DiagramsequenceDiagram
participant Config as Application Config
participant Parser as Data Parser
participant Factory as Filter Factory
participant Filter as Histogram Filter
participant GPU as GPU Compute
Config->>Parser: Parse "@style hist x y w h"
Parser->>Factory: Request createHistogramFilter()
Factory->>Filter: Instantiate & init()
Filter->>GPU: Allocate textures (256×1, 256×256)
Filter->>GPU: Load compute shaders (clear, count, draw)
GPU-->>Filter: Shaders compiled
Note over Filter,GPU: Per-frame rendering
Filter->>GPU: Execute clear pass (reset histogram)
GPU->>GPU: Zero histogram bins
Filter->>GPU: Execute count pass (compute luminance)
GPU->>GPU: Read input frame, tally luminance values<br/>via atomic adds to histogram
Filter->>GPU: Execute draw pass (render bars)
GPU->>GPU: Generate 256×256 bar-chart<br/>display texture from histogram
Filter->>GPU: Composite display texture to output
GPU-->>Filter: Histogram overlay rendered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@Auggie review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@library/src/main/jni/cge/filters/cgeHistogramFilter.cpp`:
- Around line 200-208: CGEHistogramFilter's setters accept arbitrary floats but
the API expects normalized [0,1] values; update
CGEHistogramFilter::setFormPosition and CGEHistogramFilter::setFormSize to clamp
incoming left/top/width/height into the [0.0f, 1.0f] range before assigning to
m_position and m_size (use std::clamp or equivalent), ensuring any out-of-range
inputs are constrained and the stored m_position/m_size remain normalized.
- Around line 25-33: Replace the raw CGE_SHADER_STRING(...) usages for the
inline GLSL compute sources with the repository precision helper macros;
specifically update the s_cshHistClear shader literal to use
CGE_SHADER_STRING_PRECISION_H(...) (or CGE_SHADER_STRING_PRECISION_M(...) if
medium precision is required) and do the same for the other shader literals in
the same file (the ones around the other comment ranges at lines ~39-55 and
~64-87); ensure you keep the exact shader text but wrap it with the appropriate
CGE_SHADER_STRING_PRECISION_* macro so the file follows the project's precision
macro convention.
- Line 174: The current glMemoryBarrier call only uses
GL_SHADER_IMAGE_ACCESS_BARRIER_BIT but the compute shader writes to displayImage
(binding 1) via imageStore() while later code samples that same texture via a
sampler; update the glMemoryBarrier call in cgeHistogramFilter.cpp (the call
that follows the compute imageStore to displayImage) to include
GL_TEXTURE_FETCH_BARRIER_BIT as well (bitwise OR it with
GL_SHADER_IMAGE_ACCESS_BARRIER_BIT) so texture-fetch reads see the compute
shader writes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cgeDemo/src/main/java/org/wysaid/cgeDemo/MainActivity.javalibrary/src/main/jni/cge/filters/cgeAdvancedEffects.cpplibrary/src/main/jni/cge/filters/cgeAdvancedEffects.hlibrary/src/main/jni/cge/filters/cgeDataParsingEngine.cpplibrary/src/main/jni/cge/filters/cgeHistogramFilter.cpplibrary/src/main/jni/cge/filters/cgeHistogramFilter.h
| static CGEConstString s_cshHistClear = "#version 310 es\n" CGE_SHADER_STRING( | ||
| precision highp float; | ||
| precision highp int; | ||
| layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in; | ||
| layout(r32ui, binding = 2) writeonly uniform highp uimage2D histogramImage; | ||
|
|
||
| void main() { | ||
| imageStore(histogramImage, ivec2(int(gl_GlobalInvocationID.x), 0), uvec4(0u, 0u, 0u, 0u)); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use project precision macros for inline GLSL compute sources.
Line 25, Line 39, and Line 64 currently build shaders with CGE_SHADER_STRING(...) directly. Please switch these shader literals to the required precision helper macros for consistency with repository shader conventions.
As per coding guidelines: library/src/main/jni/**/*.{cpp,h,c} requires inline GLSL via CGE_SHADER_STRING_PRECISION_H() / CGE_SHADER_STRING_PRECISION_M() macros.
Also applies to: 39-55, 64-87
🧰 Tools
🪛 Cppcheck (2.19.0)
[error] 25-25: There is an unknown macro here somewhere. Configuration is required. If CGE_SHADER_STRING is a macro then please configure it.
(unknownMacro)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@library/src/main/jni/cge/filters/cgeHistogramFilter.cpp` around lines 25 -
33, Replace the raw CGE_SHADER_STRING(...) usages for the inline GLSL compute
sources with the repository precision helper macros; specifically update the
s_cshHistClear shader literal to use CGE_SHADER_STRING_PRECISION_H(...) (or
CGE_SHADER_STRING_PRECISION_M(...) if medium precision is required) and do the
same for the other shader literals in the same file (the ones around the other
comment ranges at lines ~39-55 and ~64-87); ensure you keep the exact shader
text but wrap it with the appropriate CGE_SHADER_STRING_PRECISION_* macro so the
file follows the project's precision macro convention.
| m_drawProgram.bind(); | ||
| m_drawProgram.sendUniformf("u_scaleInv", scaleInv); | ||
| glDispatchCompute(256, 256, 1); | ||
| glMemoryBarrier(GL_SHADER_IMAGE_ACCESS_BARRIER_BIT); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n library/src/main/jni/cge/filters/cgeHistogramFilter.cpp | sed -n '160,210p'Repository: wysaid/android-gpuimage-plus
Length of output: 2108
🏁 Script executed:
grep -n "m_displayTexture" library/src/main/jni/cge/filters/cgeHistogramFilter.cpp | head -20Repository: wysaid/android-gpuimage-plus
Length of output: 342
🏁 Script executed:
grep -n "m_drawProgram" library/src/main/jni/cge/filters/cgeHistogramFilter.cpp | head -10Repository: wysaid/android-gpuimage-plus
Length of output: 226
🏁 Script executed:
grep -n "s_cshHistDraw\|initWithComputeShader" library/src/main/jni/cge/filters/cgeHistogramFilter.cpp | head -20Repository: wysaid/android-gpuimage-plus
Length of output: 354
🏁 Script executed:
sed -n '64,100p' library/src/main/jni/cge/filters/cgeHistogramFilter.cppRepository: wysaid/android-gpuimage-plus
Length of output: 1371
Add GL_TEXTURE_FETCH_BARRIER_BIT to synchronize compute image writes with texture fetches.
The compute shader writes to displayImage (binding 1) via imageStore() at line 173, but line 192 reads from the same texture via sampler. The barrier at line 174 only uses GL_SHADER_IMAGE_ACCESS_BARRIER_BIT, which synchronizes image-to-image accesses. Since the write happens via image interface and the read via texture sampler interface, GL_TEXTURE_FETCH_BARRIER_BIT is required to guarantee visibility of the compute shader writes.
🔧 Suggested fix
- glMemoryBarrier(GL_SHADER_IMAGE_ACCESS_BARRIER_BIT);
+ glMemoryBarrier(GL_SHADER_IMAGE_ACCESS_BARRIER_BIT | GL_TEXTURE_FETCH_BARRIER_BIT);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| glMemoryBarrier(GL_SHADER_IMAGE_ACCESS_BARRIER_BIT); | |
| glMemoryBarrier(GL_SHADER_IMAGE_ACCESS_BARRIER_BIT | GL_TEXTURE_FETCH_BARRIER_BIT); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@library/src/main/jni/cge/filters/cgeHistogramFilter.cpp` at line 174, The
current glMemoryBarrier call only uses GL_SHADER_IMAGE_ACCESS_BARRIER_BIT but
the compute shader writes to displayImage (binding 1) via imageStore() while
later code samples that same texture via a sampler; update the glMemoryBarrier
call in cgeHistogramFilter.cpp (the call that follows the compute imageStore to
displayImage) to include GL_TEXTURE_FETCH_BARRIER_BIT as well (bitwise OR it
with GL_SHADER_IMAGE_ACCESS_BARRIER_BIT) so texture-fetch reads see the compute
shader writes.
| void CGEHistogramFilter::setFormPosition(float left, float top) | ||
| { | ||
| m_position = { left, top }; | ||
| } | ||
|
|
||
| void CGEHistogramFilter::setFormSize(float width, float height) | ||
| { | ||
| m_size = { width, height }; | ||
| } |
There was a problem hiding this comment.
Enforce normalized bounds in histogram overlay setters.
Line 200 and Line 205 accept unrestricted values even though the API contract is normalized [0, 1]. Clamping here prevents invalid or out-of-range overlay geometry from propagating into rendering.
🔧 Suggested fix
void CGEHistogramFilter::setFormPosition(float left, float top)
{
- m_position = { left, top };
+ left = left < 0.0f ? 0.0f : (left > 1.0f ? 1.0f : left);
+ top = top < 0.0f ? 0.0f : (top > 1.0f ? 1.0f : top);
+ m_position = { left, top };
}
void CGEHistogramFilter::setFormSize(float width, float height)
{
- m_size = { width, height };
+ width = width < 0.0f ? 0.0f : (width > 1.0f ? 1.0f : width);
+ height = height < 0.0f ? 0.0f : (height > 1.0f ? 1.0f : height);
+ m_size = { width, height };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@library/src/main/jni/cge/filters/cgeHistogramFilter.cpp` around lines 200 -
208, CGEHistogramFilter's setters accept arbitrary floats but the API expects
normalized [0,1] values; update CGEHistogramFilter::setFormPosition and
CGEHistogramFilter::setFormSize to clamp incoming left/top/width/height into the
[0.0f, 1.0f] range before assigning to m_position and m_size (use std::clamp or
equivalent), ensuring any out-of-range inputs are constrained and the stored
m_position/m_size remain normalized.
2 participants
Summary
Implements
CGEHistogramFilter, a GPU-side luminance histogram overlay filter, following the same architecture as the existingCGEWaveformFilter.Closes the only genuinely new feature from PR #510 — but rewritten from scratch with a proper GPU pipeline instead of the CPU
glReadPixels-based approach in that PR.How it works
Three GLES 3.1 compute-shader passes, no CPU readback:
r32uiatomic-counter textureimageAtomicAddper pixel into the luminance binrgba8display textureThe display texture is then blended onto the frame at the configured position/size with 80 % alpha (same approach as
CGEWaveformFilter).Filter rule syntax
Parameters are normalised (0–1). Example:
Can be chained with the waveform filter:
Files changed
library/…/cge/filters/cgeHistogramFilter.hlibrary/…/cge/filters/cgeHistogramFilter.cpplibrary/…/cge/filters/cgeAdvancedEffects.h#include+createHistogramFilter()declarationlibrary/…/cge/filters/cgeAdvancedEffects.cpplibrary/…/cge/filters/cgeDataParsingEngine.cpphiststyle keywordcgeDemo/…/MainActivity.javaEFFECT_CONFIGSRequirements
imageAtomicAdd)init()returnsfalseand logs an error on ES < 3.1 devicesHow to verify
bash tasks.sh --enable-cmake --buildSummary by CodeRabbit