NightDriverStrip icon indicating copy to clipboard operation
NightDriverStrip copied to clipboard

Dev QOL fix: don't overcompile TJpg_decoder

Open robertlipe opened this issue 3 weeks ago • 1 comments

Description

Don't fetch/compile 50 copies of a lib when 2 are needed: mesmerizer and mesmerizer_lolin. Fetching, installing, building 50 copies of a JPG library for things without screens isn't a great use of developer resources.

This change moves the TJpg_Decoder library dependency from global to specifically the mesmerizer_config environment in platformio.ini. It moves the TJpg_Decoder initialization from main.cpp into src/effectmanager.cpp within the #if USE_HUB75 block, ensuring it's only included and initialized for HUB75-based environments.

Save a (very tiny) tree.

Contributing requirements

Negative bits per blinken.

  • [X] I read the contribution guidelines in CONTRIBUTING.md.
  • [x] I understand the BlinkenPerBit metric, and maximized it in this PR.
  • [x] I selected main as the target branch.
  • [x] All code herein is subjected to the license terms in COPYING.txt.

robertlipe avatar Dec 04 '25 21:12 robertlipe

I'll research this harder next week, but I think I recall the only place that the jpg decoder was ACTUALLY used was in the Mesmerizer splash screen.

Maybe my last minute change of shoving that into "all graphics libs" means it's still being over compiled, but I don't think it's wrong. Let me see if we can do better.

The recent change that pulls in all of Smartmatrix just to get one font makes things icky, but that fix is still pending. Wr get crazy things like being unable to build a use_matrix on an s3 board because Smartmatrix doesn't work on S3...or p4 or most anything but the original. All we need from SM is to extract that one font so we can display weather and stocks on spectrum-like grids.

I'll look at this again more critically. My thesis is that jpg decoder is used only by splash and thus, only on Mesmerizer and Harry's Mesmerizer. Do we agree that's our reality?

If thats the only reason we need a jpg decoder, maybe we should just reencide that image to a format that doesn't NEED another library at all.

(I'm on mobile today and Sunday, so I'm being a bit hand-wavey.)

robertlipe avatar Dec 13 '25 15:12 robertlipe

Looking at this again, I think it's right. It's not OBVIOUS, but I think it's right. And I was wrong above...

Tjpeg_decoder is used in TWO places.

  1. In splash manager. That code is ONLY on HUB75 and thus, only on the Mesmerizer twins. InitSplashEffectManager() is inside USE_HUB75. That triggers the dependency on SplashLogoEffect include/effects/strip/misceffects.h which is inside USE_HUB75.
  2. In the Weather effect to render the little icons. Weather actually renders in the the case of -DEFFECTS_FULLMATRIX (which is 2D on strips...) in m5demobase, spectrum2, and mesmerizer_config. Those pull in graphics_deps, notibably to get the gif animator. That makes that a reasonable place to put the Jpegdecoder, too.

I think that gets it applied where it's needed and only where it's needed.

If anything, these exercises make us get more honest about some of our surprising dependencies.

robertlipe avatar Dec 21 '25 21:12 robertlipe

As a (very) crude way to graph this, run pio project config | egrep -i '(env:|Tjpg|lib_deps)' then look at the projects that contain TWO occurrences (one in graphics_libs which isn't used and the key one that comes and goes in the interpolated lib_deps for lib_deps).

It's the list of the ones I named above, pluse thoat that inherit from m5demobase, and thus that get the 2D USE_MATRIX=1, which adds m5plusdemo, m5stackdemo, spectrum2. (Those were fixed in a build failure because I didn't piece that graph correctly when I read all this the first time.)

If I don't have this right (that's OK) please tell me what target is wrong - or even suboptimal - and how it's wrong.

robertlipe avatar Dec 21 '25 21:12 robertlipe

I'm sure you're right, and you made me realize we're coming from different angles. You're arguing that the inclusions are appropriate for where the decoder is actually used, and my argument is that it should be included in all matrix configurations because it could easily be used in all of them.

I know this goes against the point of only including dependencies in configurations where they are really needed. What I'm considering is that we've kind-of detached the effect set for a configuration from the rest of the config's code. From that perspective, conceptually it makes sense to show a JPEG on a non-HUB75 matrix, so I think we should allow a user to make that choice, even if the default effect sets we include in the standard don't (yet).

So, my position is this: because someone who just concerns themselves with changing the effect set for a matrix configuration without diving into the nooks and crannies you and I are familiar with, easily could decide to show a JPEG on a matrix, we should have the foundational code set up to support that happening.

rbergen avatar Dec 22 '25 10:12 rbergen

Neo, what if I told you both angled above are true because actually, they are the SAME angle?

RL: "We should build it only where it's used because I hate building code a bajillion times only to throw it away." Done. RL isn't going to intentionally submit a PR that makes RL unhappy. Seems self-evident.

RB: "We should build it on all MATRIX (2D) configurations because someone might design an effect that uses JPG." Anyone building a MATRIX configuration is probably going to search for and/or start from something that include a GIF decoder and a GIF animator. That's named "graphics_deps". That now happens to drag along a JPEG decoder too. Cool. It'll be there when they're ready. A dev is more likely to get that one line right than copying all three lines that comprised our graphics libraries, keep the version numbers in sync, etc.

An existence proof that RB gets his wish is that that little green checkmark is next to this PR. Beyond misceffects, which is kind of jammed in here for Mesmerizer, the MATRIX code already includes ONE effect that triggers TJpg:

include/effects/matrix/PatternWeather.h
49:#include "TJpg_Decoder.h"

515:            if (JDR_OK != TJpgDec.drawJpg(0, 10, icon.contents, icon.length))        // Draw the image
523:            if (JDR_OK != TJpgDec.drawJpg(xHalf+1, 10, icon.contents, icon.length))        // Draw the image

Since Weather used Jpg to draw clouds, JpegDecoder HAS to be included for all matrix builds.

Could you start a MATRIX effect today without includeing ${base.graphics_deps} in lib_deps? Maybe. There are lots of things noobs can do. We can't prevent them all, but IMO that's pretty reasonable for anyone searching the code tree. (You might like to think I torture people new to the product just so I can come in an explain things to be a hero, but that's really not how I work. I don't pinch babies so I can rock them to sleep, either. :-) IMO, we've left a pretty solid trail of breadcrumbs here to get them to a a happy place.

TBC, when I started this PR I thought that the only thing using JpgDecoder was Mesmerizer and for Splash. I think I even thought that when I submitted the PR first for an automated build. (Increasingly, especially with the situation that you know about, I'm letting Microsoft pay to build this code 44 times so my laptop doesn't burn for an hour doing it.) I got the results back, made a tweak that failed, and then it hit me what was soing on: Weather is part of EFFECTS_FULLMATRIX. Anything with EFFECTS_FULLMATRIX needs to pull in both GIF and JPG gunk for pacman and Weather. (I'm seemingly stuck in 1984: "Rock on with Pac Man Fever! The time and temperature at the tone is...")

So I started thinking that Only Mesmerizer used JPG, learned that anything using EFFECTS_FULLMATRIX uses it. Had a similar epiphany with GIF a day or two earlier, and just folded them all into a "graphics libs" thing that's brought in everywhere EFFECTS_FULLMATRIX is set. So your point wasn't where I started, but it was pretty quickly.

This means they ARE compiled only where used: They're used in HUB75 for Splasy and used in EFFECTS_FULLMATRIX by weather. Both project groups get their wishes.

How am I sure? Green checkmark. If we don't lib_dep a target that needs it, when they try to include the header, they won't be able to find it.

robertlipe avatar Dec 23 '25 05:12 robertlipe

It's very simple: with the decoder initialization code being where your PR aims to put it, non-HUB75 matrix configurations that try to show JPEGs may build but they will not work, because the callback that does the actual drawing of pixels on the matrix is never set.

That code really needs to be pulled out of the splash screen initialization function, and executed for all EFFECTS_FULLMATRIX configurations, without further conditions.

I'd say: leave it where it is (in main.cpp) and wrap an #if EFFECTS_FULLMATRIX directive around it, or put it in a more effects-focused place (effects.cpp, or effectsmanager.cpp if you really prefer) that is executed for all EFFECTS_FULLMATRIX configurations as well.

rbergen avatar Dec 23 '25 08:12 rbergen