fix(webview): avoid black flash when an image follows a video

[Glassto]

Issues Fixed

Fixes #3262

Description

Instead of leaving the canvas blank for the full QNetworkReply round-trip, fall back to the last real raster frame while a fresh loadImage() fetch is in flight. Only applies to that specific gap, the intentional blanks in playVideo() and loadPage() are unchanged.

Checklist

• I have performed a self-review of my own code.
• New and existing unit tests pass locally and on CI with my changes.
• I have done an end-to-end test for Raspberry Pi devices.
• I have tested my changes for x86 devices.
• I added a documentation for the changes I have made (when necessary).

[vpetersson-bot]

Reviewed by reading the full surrounding code, not just the diff. Hardware validation on the Pi 4 testbed is running in parallel and I will follow up with measurements.

Overview

Good approach and genuinely well-commented — the comments explain why each blank is or isn't eligible for the fallback, which is exactly what this file needs. The design is right: rather than trying to shorten the fetch, keep the last decoded frame and gate the fallback on a flag so the three intentional blanks (playVideo(), the "null" sentinel, loadPage()) stay pure black. The const QImage &imageToPaint = cond ? a : b; binding is correct C++ — both operands are lvalues of the same type, so the conditional is an lvalue and the reference binds directly with no temporary.

Main issue: the flag is never cleared on failure

fallbackToLastImageOnBlank is set true when a real fetch starts, but cleared only on success — in loadAsStaticImage's success branch and in setupAnimation(). Three terminal failure paths leave it true:

1. finished handler, reply->error() != NoError → only qDebug()
2. errorOccurred handler → only qDebug()
3. loadAsStaticImage's else (decode failure) → only qDebug()

None of those assigns currentImage, so it stays null from the preceding playVideo() blank. paintEvent's condition currentImage.isNull() && fallbackToLastImageOnBlank therefore stays true, and the previous image keeps painting until the next asset rotates in.

So an image asset that 404s, times out, or is corrupt — arriving right after a video — now shows the previous asset instead of black. That is worse than the black it replaces, because a stale frame is indistinguishable from success: an operator looking at the screen sees rotation apparently working. The viewer already has machinery specifically for noticing unreachable assets (_asset_is_displayable, _trigger_asset_recheck), and this would visually hide the symptom they'd report.

Suggested fix — one line. QNetworkReply::finished is emitted for failed replies too, so it is a single choke point for every terminal outcome. Clearing the flag just after the staleness guard covers all three paths, and the success branch harmlessly clears it again:

connect(reply, &QNetworkReply::finished, this, [this, reply, requestId]() {
    reply->deleteLater();

    if (requestId != loadGenerationId) {
        qDebug() << "Ignoring stale image response";
        return;
    }

    // This request is over, whatever the outcome. Drop the fallback so a
    // failed fetch goes black rather than leaving the previous asset on
    // screen, which would be indistinguishable from success.
    fallbackToLastImageOnBlank = false;
    ...

Memory cost, worth making a conscious choice

lastRasterImage is never cleared, so it retains a decoded buffer for the life of the process. QImage is copy-on-write, so lastRasterImage = nextImage shares rather than copies — but once currentImage moves on to the next image, the old buffer is kept alive solely by lastRasterImage. Net cost is one extra full-resolution buffer, permanently.

At the ~2.07 MP cap that low-RAM boards now downscale to, that is roughly 8 MB ARGB32. On the 787 MB boards that's tolerable; on the 512 MB Pi 3 A+ (~361 MB usable, and measured sitting at swap exhaustion while idle) it is more meaningful. It cannot simply be freed in playVideo() — having it after a video is the entire point — so this is the genuine price of the feature rather than a bug. I'm measuring the actual delta on hardware and will post the numbers; flagging it so it's a decision rather than a surprise.

Test coverage

src/anthias_webview/tests/tests.pro deliberately excludes view.cpp — its comment notes rotation.cpp was kept QtCore-only precisely so the tests link without QtWebEngine. So view.cpp is in no test build, and the checklist's "existing unit tests pass" is true but doesn't exercise any of this.

Constructive suggestion following the repo's own precedent: the new decision is a pure predicate — given (currentImage.isNull(), fallbackToLastImageOnBlank), which image should paint? Extracting that into a small QtCore-only helper alongside rotation.cpp would make it unit-testable without WebEngine, and would let the failure-path behaviour above be pinned by a test rather than by review.

Qt5 coverage question

playVideo() — and therefore its fallbackToLastImageOnBlank = false — is inside #if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0). On the armhf Qt5 boards (Pi 2, Pi 3 32-bit) that clear never compiles, so those boards reach the correct state only via the "null" sentinel in loadImage(). In practice the viewer does call view_image('null') before a video, so it should hold — but it makes the Qt5 path depend entirely on that call always being made, which is worth a comment at minimum.

Related, and I'd genuinely like your read rather than asserting it: on Qt5 the video is played by an external gst_fbdev_player writing to /dev/fb0, not by the webview. Is there a window where the webview could repaint lastRasterImage over a still-playing video's framebuffer during the handover? The Qt6 eglfs/Wayland path doesn't have this shape.

Which board and stack did the "end-to-end test for Raspberry Pi devices" tick cover? Qt5 and Qt6 are materially different paths here, so it would be useful to know which one was exercised.

Minor

• setupAnimation() sets lastRasterImage to the first GIF frame only, not the current frame as the animation runs. That's fine for the purpose and the header comment says so accurately — just noting it's intentional.
• No security surface: no new input handling, the decode path is unchanged (QImageReader with setAutoTransform), no new network exposure.
• Performance impact on paintEvent is one null check plus a reference bind. Negligible, and no extra decode.

Summary

The core idea is sound and I'd like to see it land. The failure-path flag leak is the one thing I'd consider blocking, since it converts a visible error state into a silent one — and it's a one-line fix. The memory retention and the missing test are worth a decision and a follow-up respectively rather than blocking. Hardware numbers to follow.

[Glassto]

Thanks for the thorough review, especially for pushing on the Qt5 handover question rather than letting me hand-wave past it.

Flag leak on failure: fixed exactly as suggested, cleared in the finished handler right after the staleness guard, before checking error(), so it's a single choke point for all three terminal paths (network error, errorOccurred, decode failure). A failed/corrupt fetch now goes black again instead of leaving the previous asset up.

Test coverage: extracted the predicate into image_fallback.{h,cpp}, same shape as rotation.cpp, QtCore-only, no WebEngine. test_image_fallback.cpp pins the three cases: non-null currentImage never falls back, the intentional blanks stay black, and a real fetch from a blanked state uses the fallback. Wired into tests.pro and the combined test runner.

Qt5 / gst_fbdev_player question: traced this through rather than asserting. GstFbdevMediaPlayer.stop() (media_player.py) blocks on Popen.wait(), SIGTERM then SIGKILL fallback, until the subprocess has actually exited, and view_video() doesn't return to asset_loop until that stop() call returns. So the next view_image(), and therefore any repaint of currentImage/lastRasterImage onto /dev/fb0, can't happen until gst_fbdev_player has confirmed-exited and stopped writing frames. No cross-process lock enforces this, it's an ordering guarantee from the Python call sequence, unchanged by this PR. Added a comment above playVideo() documenting the dependency, since you're right it's non-obvious from the #if QT_VERSION >= 6 guard alone.

Memory: agreed it's worth a conscious call rather than silently accepting it. It's bounded to one extra buffer at a time (lastRasterImage is reassigned on every successful load, not accumulated), so I'd lean toward accepting the cost as-is unless your hardware numbers say otherwise on the 512 MB boards, in which case I'm open to gating the fallback off entirely below some RAM threshold. Will wait for your measurements before deciding.

Board tested: pi4-64 (Qt6/eglfs). I have not yet tested the Qt5/linuxfb path (Pi 2, Pi 3 32-bit) or the Qt6/Wayland path (Pi 5, x86) on real hardware,the Qt5 analysis above is from reading the code, not from running it on an armhf board.

Updated patch pushed to the branch..... :)

[Glassto]

Self-review pass on the full diff (not just the delta from the previous version).

• view.h / view.cpp: confirmed fallbackToLastImageOnBlank is reset in every place that blanks currentImage (loadImage's null sentinel, loadPage, playVideo) and cleared unconditionally in the QNetworkReply finished handler before the success/failure branch,
• image_fallback.{h,cpp}: pure function, no state, matches the shape of rotation.cpp.
• test_image_fallback.cpp: covers all three cases from the review discussion, ran locally via bin/test_webview_cpp.sh, all green;.
• Comment above playVideo() accurately describes the Qt5/gst_fbdev_player ordering dependency, double-checked against media_player.py's stop() implementation.

No leftover debug output, no unrelated changes. Marking this Comment(can't approve my own PR) since the self-review checklist item is genuinely done now, not just checked off.

[vpetersson-bot]

Second pass on the updated patch. I read the full loadImage() / paintEvent() flow again rather than just the delta, traced the Qt5 ordering claim through __init__.py and media_player.py, and built and ran the new test locally.

First, two corrections to my own previous review

The memory cost I flagged is essentially zero, and I was wrong about it. I claimed lastRasterImage keeps "one extra full-resolution buffer, permanently". It does not, on the static-image path. nextImage (view.h:100) is assigned exactly once, at view.cpp:925, and is never cleared anywhere in the class. It already retained the last decoded static image for the life of the process, before this PR. Since loadAsStaticImage() does nextImage = newImage; currentImage = nextImage; lastRasterImage = nextImage, all three are copy-on-write aliases of the same buffer, and all three are reassigned together on the next successful load. So there is no additional retention to reclaim.

The one place a genuinely new buffer is retained is setupAnimation() (view.cpp:1114): lastRasterImage = currentImage takes a movie->currentImage() frame that nextImage does not hold. That is bounded at one frame, and only after an animated GIF.

So: no need to gate the fallback on a RAM threshold, there is nothing meaningful to save. Please drop that option rather than carrying it as an open question.

On the hardware numbers I said I would post: I do not have them, and I am not going to imply otherwise. I did not run the Pi 4 or Pi 3 A+ testbed this round. Given the aliasing above, the static-path measurement would not have been informative anyway, the question is answered by reading the lifetimes. If you want a number for the GIF path specifically that is worth a real measurement, but it should not block this.

Consequence for the patch: the memory paragraph in the view.h comment on lastRasterImage now enshrines my incorrect claim ("it does keep one otherwise-dead decoded frame alive for the rest of the process's life once currentImage moves on (roughly 8 MB ARGB32 ...)", "Worth watching on the 512 MB Pi 3 A+ specifically"). That is the most load-bearing kind of comment to get wrong, someone will eventually try to reclaim a buffer that is not there. Please rewrite it to say what is actually true: no marginal cost on the static path because nextImage already pins the same buffer, one extra frame after a GIF.

The flag leak is properly fixed

Clearing at view.cpp:854, after the staleness guard and before the error() branch, is exactly right. finished is emitted for failed replies too, so all three terminal paths (network error, errorOccurred, decode failure in loadAsStaticImage's else) are covered by the one assignment. Placing it after the generation check is also correct and worth noting deliberately: a late stale reply must not clear the flag belonging to a newer in-flight request. Easy to get backwards, you got it right.

Verified, not just eyeballed

• Built and ran the new test. Compiles clean under -Wall -Wextra with no warnings, 6 passed / 0 failed (Qt 6.10.2). I built image_fallback.cpp + test_image_fallback.cpp standalone against QtCore + QtTest with a shim main(), since the full tests.pro pulls multimedia/quick. The runImageFallbackTests factory plus #include "test_image_fallback.moc" wiring works, and INCLUDEPATH += ../src was already there so the image_fallback.h include resolves.
• The fallback actually fires. I checked this because it would silently no-op if it did not: fallbackToLastImageOnBlank = true (view.cpp:798/815) is set after hideVideoSurface() (view.cpp:732), which is what exposes this widget and triggers the repaint. hideVideoSurface() uses setVisible(false), not repaint(), so the repaint is deferred to the event loop and the flag is already true by the time paintEvent() runs. Correct as written, but it is load-bearing on Qt never repainting synchronously there. Setting the flag before the hideVideoSurface() call would cost nothing and remove the dependency.
• No rotation artifact. I expected a problem here and there is not one: imageRotation is set once in the constructor (view.cpp:278) from rotation::linuxfbRotationOverride(), so it is a screen-level constant, not per-asset. No risk of painting the stale frame at an incoming asset's rotation.
• Your Qt5 conclusion is sound. stop() does killpg(SIGTERM) then wait(timeout=3), escalating to SIGKILL (media_player.py:855-878), and view_video() does not return until it completes (__init__.py:1527). The flag can only go true inside a later real loadImage(), so it cannot be armed while the fbdev writer is live. The reasoning holds.

The new Qt5 comment contradicts the comment 10 lines above it

view.cpp:743-745 says the fallback

depends on view_video() always sending it before media_player.play()'s asset actually starts, which it does today (__init__.py view_video()).

The actual order in view_video() is media_player.play() at __init__.py:1509, then view_image('null') at __init__.py:1511. Play comes first.

And the pre-existing comment at view.cpp:718-729, in the same function, already states this correctly and in capitals: "view_image('null') ... is called AFTER media_player.play()". So the patch adds a comment that contradicts one visible on the same screen.

I think the intent behind the new wording was "the blank lands before the first video frame paints", which is probably true in practice (the helper is a fresh Python process importing GStreamer and negotiating caps, seconds on a Pi 3) but that is a race the code wins, not an ordering guarantee, and it is not what the sentence says. Since this comment exists specifically to warn whoever next touches the Qt5 path, having the direction backwards defeats its whole purpose. Please restate it around the guarantee that actually holds, the one in your third bullet: the flag only arms in a later loadImage(), and stop() has already blocked before the next asset is dispatched.

The test does not pin what its comment says it pins

The header of test_image_fallback.cpp claims it

pins the behaviour from the PR review discussion directly: a failed/corrupt image fetch must NOT leave the previous asset on screen (fallbackAllowed cleared on every terminal QNetworkReply outcome in View::loadImage(), not only on success)

It does not, and cannot. The extracted predicate is currentImageIsNull && fallbackAllowed. The tests assert true && false == false and true && true == true. The behaviour that was actually at risk, and that my previous review flagged, is the lifecycle of the flag: which code paths set it and which clear it. All of that still lives in view.cpp, which tests.pro still cannot link.

My suggestion to extract the predicate is what produced this shape, so this is partly on me. The extraction is still worth keeping, it makes paintEvent() read better and matches the rotation.cpp precedent. But please cut the coverage claim down to what the file does, because the next person to reorganise the flag assignments will read that paragraph and believe a test is guarding them when nothing is.

Related and purely factual, not an ask: these C++ tests do not run in CI. bin/test_webview_cpp.sh says so itself ("CI integration is a follow-up") and nothing under .github/ references test_webview_cpp, tests.pro, or AnthiasViewerTests. So the checklist's "New and existing unit tests pass locally and on CI" is accurate only for the Python suite. Pre-existing gap, not this PR's job to close, but it bounds how much the new test buys: it runs when someone remembers to run it.

Scope is wider than the PR description says

The PR body says the fallback "Only applies to that specific gap", the post-video one. It does not. fallbackToLastImageOnBlank = true is set on every real loadImage() (view.cpp:798 and 815), and loadPage() blanks currentImage (view.cpp:524). So a webpage-to-image transition also gets the fallback.

That case behaves differently from the video case in a way worth a decision:

• lastRasterImage is the last successfully decoded image, not the last asset. For a playlist like [imageA, video, webpage, imageB], the webpage -> imageB transition paints imageA, which is two assets and potentially minutes old.
• More concretely: an image asset that has since been deleted or disabled in the playlist can briefly reappear on screen, because nothing invalidates lastRasterImage when an asset goes away.

Both are brief and neither is a correctness bug, and a stale frame may well still beat a black one. But it is a different claim than the PR makes, and the deleted-asset flash is the kind of thing that generates a confused forum post. Two honest options: accept it and describe it accurately in the PR body and the view.h comment, or arm the flag only for the blanks you actually intend to cover. I lean toward the former, it is simpler and the window is short, but it should be a decision rather than an unnoticed side effect.

Minor

• view.cpp:851, "The success branch below re-arms lastRasterImage/the flag". The success path sets the flag to false, it does not re-arm it. "Re-arms" reads as "sets true". Suggest "clears it again (harmlessly)".
• The Qt5 comment says stop() blocks "until that external process is confirmed gone". Almost true: if SIGKILL plus a 3 second wait() also times out, media_player.py:870-875 swallows the exception and clears self._proc regardless. Practically unreachable for a SIGKILLed process group, but "confirmed gone" is a shade stronger than the code guarantees.
• Drive-by, pre-existing, ignore if you would rather not touch it: the comment at view.cpp:718 points at src/anthias_viewer/__init__.py:495, but view_video() is at line 1504 now.
• No new security surface. No new input handling, decode path unchanged (QImageReader with setAutoTransform), no new network exposure.
• paintEvent() cost is one extra null check and a reference bind. The const QImage & binding to the conditional is still correct, both operands are lvalues of the same type.

Summary

The blocking item from last round is genuinely fixed, and fixed at the right place. What is left is documentation accuracy rather than behaviour, but three of the comments this PR adds or that it now backs make claims that are not true (the memory cost, the Qt5 call order, the test's coverage), and comments are the entire reason this code will be maintainable in a year. Worth one more pass on the prose.

The memory question is closed, do not gate on RAM. On test coverage I would accept what is here with a corrected comment. The scope question needs a call from you, not more analysis from me.

Still worth landing.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Glassto]

Pushed. Going through in the order you raised things.

Your two self-corrections, I agree with you on both. No RAM gating, dropped it as an option, nextImage already pinned that buffer before this PR existed. Rewrote the view.h comment to say that plainly instead of the ~8 MB claim. Appreciate you catching your own claim before I built on top of it.

Qt5 comment: fixed. You're right, play() runs before the sentinel, not after, and I had it backwards. Restated around the guarantee that actually holds, the flag only arms in a later real loadImage(), and stop() has already blocked before that can happen, not around the play()/null ordering.

Test coverage comment: narrowed it to what the file tests, the pure predicate, and said explicitly that the flag lifecycle (which paths set/clear it) isn't covered, since that's still in view.cpp. Kept the extraction, per your earlier suggestion, just stopped overclaiming what it guards.

"re-arms" wording: modified to "clears it again."

hideVideoSurface() ordering: moved the flag assignment before the call, so it's not resting on setVisible()'s repaint deferral anymore. Free, as you said.

Scope: going with your option (a), keeping it broad rather than restricting to the video case. Documented in the view.h comment now: any real loadImage() arms the fallback, so webpage-to-image gets it too, and lastRasterImage can be an asset from further back than the immediately preceding one. Decided a short-lived stale frame beats black here, same call as the video case, and the window's bounded by one round-trip either way.

Also took the two minor ones: comment now points at init.py:1504 instead of the stale :495, and "confirmed gone" is now "has exited," matching what wait() actually guarantees.

Ran bin/test_webview_cpp.sh again after all of this, still green. Didn't touch the CI gap, agreed that's pre-existing and out of scope here.

[Glassto]

Self-review on the updated diff.

Confirmed fallbackToLastImageOnBlank now sets before hideVideoSurface(), not after, and the Qt5 comment no longer claims an ordering it doesn't have. view.h's memory note matches what's actually true now (no marginal cost on the static path). Test file's header only claims what it tests.

Nothing left unaddressed from either round. Good to go from my side... :)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (master@936e7d8). Learn more about missing BASE report.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #3275   +/-   ##
=========================================
  Coverage          ?   90.65%           
=========================================
  Files             ?       76           
  Lines             ?     8467           
  Branches          ?      898           
=========================================
  Hits              ?     7676           
  Misses            ?      570           
  Partials          ?      221           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.