[rafalmaciag]

HttpFrameGrabber could not give a frame together with its own count. PublishFrame swapped _latest under _frameLock but incremented _frameCount after the lock. So no read order from outside could pair them: a new frame could be read under the previous count, and then again under its own. One frame then looks like two to a consumer that tells frames apart by count. Found in rocket-welder2 PR 444 review (S-11): rw2's settle gate uses the count to tell frames apart (rw2 #441).

Change

• PublishFrame stores the frame and its count together, inside the lock (_latestCount = Interlocked.Increment(ref _frameCount) next to _latest = handle).
• New JpegFrameHandle? TryAcquireLatest(out int frameCount) returns the handle and the count it was published under, from one locked read. TryAcquireLatest() now delegates to it.
• Unchanged: FrameCount, HasFrame and TryAcquireLatest(), in signature and meaning. FrameCount's doc now says that, read on its own, it does not identify the frame TryAcquireLatest() returns.
• For the tests: PublishFrame becomes internal, plus an internal hook AfterSwapForTest and InternalsVisibleTo the test project.

Consumers checked

In rocket-welder2, LiveRawFrameSource is the only reader of FrameCount/TryAcquireLatest. CameraStreamService mentions the grabber in a comment and reads CameraDevice.FrameCount, a different counter. No other repository in the workspace index uses HttpFrameGrabber (checked: rocket-welder-sdk, streamer, native-player, platform).

Moving the increment inside the lock changes nothing visible to them:

• FrameCount is still a volatile read of the same counter.
• The lock is held for one more interlocked increment.
• _hasFrame is still set after the publish.

Tests

• HttpFrameGrabberFrameCountTests (new, no network):
  • The publish of frame 2 is held right after the swap while another thread reads; the reader must get (frame 2, count 2). With the fix, the reader waits for the publish to finish.
  • A sequential check that each frame is read with its own count.
• Red: today's pairing behind the same signature (count read before the frame, increment after the lock, hook between swap and increment) fails the interleaving test.
• Non-integration suite: 113/113 passed (Debug, net10.0, on tycho).
• Integration tests not run green here: the Category=Integration tests (HttpFrameGrabberTests, MjpegProviderTests) need a live MJPEG stream on 127.0.0.1:4953. Here they fail with connection refused, as they would on master with no stream.

Review S-14 (head 2980e37)

• MjpegProvider, the shared feed rocket-welder2's CameraDevice is built on, gains TryGetFrame(out int frameCount) and TryAcquireRaw(out int frameCount). Both go through HttpFrameGrabber.TryAcquireLatest(out int). The existing TryGetFrame(), TryAcquireRaw() and FrameCount are unchanged.
• Tests:
  • the provider reads a raw frame with its own count while a publish is held after the swap;
  • the provider decodes a frame (TestData JPEG) with its own count.
• Red with today's pairing (the provider reading FrameCount, then the frame, over today's grabber): the provider interleaving test fails, as does the grabber's.
• Non-integration suite: 115/115.

Release

None in this PR. Publishing a new version needs the owner's word. rocket-welder2 then bumps 1.4.1 to the new version (which also brings 1.5.0's Rgb24 codec) and moves LiveRawFrameSource to TryAcquireLatest(out int).

🤖 Generated with Claude Code

[rafalmaciag]

mjpeg PR 4: a frame and its count in one locked read (PR 444 S-11)

Verdict: APPROVE at 05a78c7, with one Should: MjpegProvider, rw2's shared feed, still pairs separately.

• PR: mjpeg #4, head 05a78c7, from master 04f8f2d (tag v1.5.0). 2 commits, 3 files, +87 / −6.
• Reading only. Evidence someone else ran: non-integration 113 of 113. The integration tests need a live stream on 127.0.0.1:4953 (connection refused, as on master). The new interleaving test is red with today's pairing.

Is every writer of _latest / _frameCount under the lock? Yes.

• PublishFrame swaps _latest and now sets _latestCount = Interlocked.Increment(ref _frameCount) inside lock (_frameLock) (HttpFrameGrabber.cs:413-420).
• DisposeAsync and Dispose clear _latest under the same lock (:450-455, 475-480).
• _frameCount has no other writer. There is no reconnect or reset path: the parse loop ends with the stream (:127-160), and a new grabber is a new object.
• FrameCount stays a lock-free volatile read, which is fine for its own meaning.
• _latestCount is not reset on dispose. That is harmless: _latest is default, so TryAcquireLatest answers null and 0.

Can a reader see its handle disposed by the publisher? No. The lifetime is unchanged.

• The reader TryAddRefs under the lock (:110-118).
• The publisher disposes the old handle outside the lock, which releases one reference; the reader's own reference keeps the buffer.
• Dispose works the same way.

Is the lock held across anything slow? No.

• JpegFrameHandle.Rent and CopyTo are before the lock, and old.Dispose() is after it.
• Inside the lock: two field writes, one Interlocked.Increment, and the test hook, which is null in production (a null check per frame).

The interleaving test is deterministic.

• AfterSwapForTest runs inside the lock right after the swap. It starts a reader that calls TryAcquireLatest(out int), which must wait for the lock, and waits up to 500 ms for it.
• With the fix the reader cannot finish until the writer has incremented and left the lock, so it reads (2, frame 2) every time. The 500 ms only bounds the wait; it decides nothing.
• With today's pairing (count read first, then the frame) the reader reads count 1 before blocking and then frame 2: red every time.

The test seam.

• internal Action? AfterSwapForTest, PublishFrame made internal, and InternalsVisibleTo("ModelingEvolution.Mjpeg.Tests") (.csproj:19-21).
• None of it is in the public surface. The production cost is the null check per frame.

Public API: nothing breaks.

• A new overload TryAcquireLatest(out int frameCount) is added. TryAcquireLatest() now delegates to it, with the same behaviour. FrameCount is unchanged.
• Consumers I could find through the repository index plus a per-repository git grep (no recursive workspace search):
  • rw2's LiveRawFrameSource (FrameCount + TryAcquireLatest());
  • mjpeg's own MjpegProvider (FrameCount => _grabber.FrameCount, _grabber.TryAcquireLatest() at MjpegProvider.cs:58, 78).
  • rw2's CameraStreamService and the simulator's CameraPreviewGateway only mention it in comments or read CameraDevice.FrameCount.
  • Checked: rocket-welder2, rocket-welder-sdk, streamer, native-player, simulator, FroniusCanbus.

Versioning.

• The csproj carries Version 1.0.0; releases are set by tag (latest v1.5.0).
• This would be v1.5.1, a compatible addition. No release is ordered: it needs the owner's word.
• rw2 then needs a package bump and LiveRawFrameSource.TryGrab switched to the new overload. PR 448's comment says so.

Conventions: met. No id, event or wire type, and one private int field under the existing lock (use-thread-safe-collection: correct use of the one lock). Unsourced constraints: none.

Should

S-14. The shared camera feed keeps S-11's race after this PR.

• MjpegProvider (MjpegProvider.cs:58, 78) reads _grabber.FrameCount and _grabber.TryAcquireLatest() separately. rw2's shared feed reads it the same way: CameraDevice.FrameCount → MjpegProvider.FrameCount, and CountedFrame.FromFeed reads the count, then TryGetImage.
• My PR 444 S-11 named both LiveRawFrameSource.TryGrab and CountedFrame.FromFeed. This PR fixes the grabber's pairing and gives rw2's raw path a way to use it. The feed path needs MjpegProvider to offer the same paired read, for example a decoded frame with the count it was published under.
• Without that, after 1.5.1 the auto-adapt dialog, teach drive-to-view and the walk's fallback stay on the two-read pairing. The effect is bounded, as reported in S-11.

[rafalmaciag]

mjpeg PR 4 re-read at 2980e37 (S-14)

Verdict: APPROVE at 2980e37. S-14 is closed on the mjpeg side.

• Range: git diff 05a78c7..2980e37. 2 commits, 2 files, +65 / −4.
• Reading only. Evidence someone else ran: non-integration 115 of 115; both new interleaving tests red with today's pairing.
• No merge or release is ordered; both wait for the owner's word.

The paired reads on MjpegProvider:

• TryAcquireRaw(out int frameCount) is _grabber.TryAcquireLatest(out frameCount) (MjpegProvider.cs:119-120): one locked read.
• TryGetFrame(out int frameCount) (:83-105):
  • the count comes from the same TryAcquireLatest(out frameCount) call that returns the handle, and that handle is what gets decoded (_codec.Decode(handle.Value.Data, …)). So it is one frame end to end;
  • the handle is released in finally on every path: the normal return, a GetImageInfo or Decode exception, and the null return before the try (nothing acquired).
• An existing trait, not new here: when Decode throws, the MemoryPool buffer rented for that frame is not returned to the pool (no owner.Dispose() in a catch). The garbage collector reclaims it; the pool only misses one buffer. Same as before the PR.
• Public API: additions only. TryGetFrame() and TryAcquireRaw() keep their signatures and behaviour (TryGetFrame() delegates with out _), and FrameCount is unchanged. The cref fix in 2980e37 is documentation only.
• Tests:
  • the raw read is held after the swap, as for the grabber;
  • a TestData JPEG (rgb24-reference.jpg) is decoded with its own count over three publishes, with null and 0 before the first.

What rw2 must change after the bump to ModelingEvolution.Mjpeg ≥ 1.5.1, so both feeds use the paired read. These are the call sites on rw2 master 301e7fdb:

#	Where	Today	Change
1	LiveRawFrameSource.cs:58-59 (raw path)	var count = _grabber.FrameCount; then _grabber.TryAcquireLatest()	_grabber.TryAcquireLatest(out var count), and correct the comment at :52-55 again
2	CameraDevice.cs:162-163 (TryGetImage)	freshness from _provider.FrameCount, then _provider.TryAcquireRaw()	_provider.TryAcquireRaw(out var count), with FrameIsFresh(count) on that same count
3	CameraDevice gains a counted grab	only FrameCount (:44-50) and TryGetImage	Expose a counted image: for example CountedFrame? TryGetCountedImage() returning the decoded MatFrame with the count from #2. Or move IFrameCounter's contract to "a counted grab" rather than "a count"
4	CountedFrame.FromCamera / FromFeed (Automation/Vision/CountedFrame.cs:18-33)	FromFeed(tryGetImage, frameCount) reads the count, then the image	Take the camera's counted grab from #3, so FromCamera returns it. FromFeed's two-delegate shape goes, or remains for test fakes only
5	AdaptViewWalkFactory.cs:217 (walk fallback)	CountedFrame.FromFeed(camera.TryGetImage, counter)	CountedFrame.FromCamera(camera)
6	GuidedRun.cs:769 (CountedGrab fallback) and its port FrameCounterOf (:73, 85)	CountedFrame.FromFeed(_camera.TryGetImage, counter)	CountedFrame.FromCamera(_camera). The FrameCounterOf port can then go
7	AdaptationServiceExtensions.cs:69, 86, 104-105	registers FrameCounterOf for the walk factory and GuidedRuns	Delete it with #5 and #6, or keep it only if the factory still needs it
—	MakeAdaptiveDialog.razor:677, RunScreenHost.razor:553	CountedFrame.FromCamera(camera)	No change once #4 is done

• The test fakes implementing IFrameCounter or calling FromFeed, in Cobot.Tests and RocketWelder.Tests, follow LibJpegWrap follow-ups from the review of #2 (Rgb24 codec) #3 and HttpFrameGrabber: read a frame with its own count (TryAcquireLatest(out int)); frame and count change together #4.
• No setting changes.
• S-11's comment in LiveRawFrameSource becomes false once fix: rebuild linux-arm64 LibJpegWrap.so with decoder #1 lands, and should then say "one locked read".

[rafalmaciag]

Merged on an approved review and a side-by-side run of the CI workflow's own steps on tycho (2026-11-13): master 04f8f2d 130 tests, 114 passed, 16 failed; this PR's head 2980e37 134 tests, 118 passed, 16 failed. The 16 failing tests are identical on both, all Category=Integration: 7 need a live MJPEG stream on 127.0.0.1:4953 ("Connection refused"; the repo provides no source for it and the workflow starts none), 9 cannot load Emgu.CV's native library (libXt.so.6 and libusb-1.0.so.0 missing; with libxt6 and libusb-1.0-0 installed all 9 pass on both commits). The 4 tests this PR adds pass. So the CI check is red on master for reasons this PR neither causes nor changes; that is why CI was not the gate here.

For the repo's owner, not done here: the workflow should either filter out Category=Integration or provide the stream, and install the two libraries.

🤖 Generated with Claude Code