livestream: read frames with readexactly to avoid truncation - #1232
livestream: read frames with readexactly to avoid truncation#1232jasoncarreira wants to merge 1 commit into
Conversation
|
Thanks a lot, this looks good! You mentioned that you are working on HA integration as well, have you seen my work-in-progress PR which was blocked due to performance issues and the recent changes to the blink API? I would like to avoid double-work. |
|
@jasoncarreira this looks good- just fix the failing test (probably the test is expecting a certain call and your change modified that flow) and I'll merge. |
StreamReader.read(n) returns up to n bytes, so a header or payload split across TCP segments was misread as a short read and aborted the stream. readexactly() blocks for the full frame and raises IncompleteReadError only on a genuine EOF. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d69332a to
58fc442
Compare
|
Thanks @mback2k! No double-work to worry about — I'm not doing a competing HA-core PR. My Blink live-view work was just a local A few findings from reverse-engineering the immis stream that may help the perf/stability issues:
I've got several Blink cameras + a doorbell on HA — happy to test #160708 and give performance feedback. |
From previous experiences with other HA core integrations the core developers will probably ask the library to take care of that, for example via a mutex lock or something similar inside the library API instance. |
|
I hit this independently and opened #1262 before spotting this PR, sorry for the noise. Since I had already dug into the CI failure, here is what is blocking the merge. The CI blockerThe only red check is All four are the same shape, a mock_reader.readexactly.side_effect = [header_data, payload_data, asyncio.IncompleteReadError(b"", 9)]
A regression test you may wantThe current tests all mock The subtlety is that the payload has to arrive while async def test_recv_payload_split_across_segments(self, mock_resp):
"""Test that a payload split across TCP segments is reassembled."""
header_data = bytearray([0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0xBC])
payload_data = bytearray([0x47] + [0x00] * 187) # 188 bytes
# Only the first segment is buffered when recv() starts, so the payload read
# finds fewer bytes than the header promised. The rest arrives while recv()
# is already waiting on it.
reader = asyncio.StreamReader()
reader.feed_data(header_data)
reader.feed_data(payload_data[:100])
mock_client = mock.Mock()
mock_client.is_closing.return_value = False
mock_client.write = mock.Mock()
mock_client.drain = mock.AsyncMock()
self.livestream.target_reader = reader
self.livestream.target_writer = mock.Mock()
self.livestream.clients = [mock_client]
recv_task = asyncio.create_task(self.livestream.recv())
await asyncio.sleep(0) # let recv() drain the buffer and block on the rest
reader.feed_data(payload_data[100:])
reader.feed_eof()
await recv_task
mock_client.write.assert_called_once_with(payload_data)On Happy for that to be taken as-is, or I can open a PR into the branch if that is easier. Either way, thanks for the fix, it is the difference between live view working and not on a camera with a marginal signal. |
Description
BlinkLiveStream.recv()reads the 9-byte immis frame header and then thepayload with
StreamReader.read(n):StreamReader.read(n)returns up tonbytes — it resolves as soon as anydata is buffered. When a header or payload is split across TCP segments (very
common for the larger video payloads),
read()returns a short buffer, thelen(data) < nguard treats it as EOF, and the stream is torn down mid-frameeven though the connection is perfectly healthy.
This replaces both reads with
readexactly(), which waits for the full frameand raises
IncompleteReadErroronly on a genuine EOF:How I found it
While getting live view working through Home Assistant I built a standalone
relay that parses the immis framing from a buffer (accumulate-then-slice) and it
streamed hundreds of H.264 frames reliably. A faithful port that used
read(n)instead would intermittently abort with "Insufficient data". The difference is
exactly this partial-read handling.
Note: this is independent of the recent OAuth/2FA (202) and liveview-endpoint
work in #1227/#1228/#1229/#1231 — it's a latent framing bug that surfaces once a
live view actually connects and starts delivering video.
Checklist
read(n)truncates)tox(no test currently exercisesrecv()framing; happy to add one if desired)