labels rendering performance improvement: create ImageBitmaps in worker #5169
Conversation
WalkthroughThe changes in this pull request involve significant refactoring of the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@sashankaryal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
sashankaryal
force-pushed
the
improv/image-bitmaps
branch
2 times, most recently
from
69d495a to
a782eff
Compare
sashankaryal
force-pushed
the
improv/image-bitmaps
branch
from
a782eff to
a38a8b0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (18)
app/packages/looker/src/worker/shared.ts (2)
Line range hint
6-12: Add TypeScript type annotations for better type safety.The function lacks TypeScript type annotations which could lead to type-safety issues.
Consider applying this improvement:
-export const mapId = (obj) => { +export const mapId = <T extends { _id?: any; id?: any }>(obj: T): T => { if (obj && obj._id !== undefined) { obj.id = obj._id; delete obj._id; } return obj; };
14-21: Add JSDoc documentation and improve type safety.While this function is part of the labels rendering performance improvement, it could benefit from better documentation and type safety.
Consider these improvements:
+/** + * Returns the overlay field configuration based on the class type. + * Used in the worker thread for efficient overlay data processing. + * @param cls - The class type of the overlay + * @returns Object containing canonical and disk field names + */ -export const getOverlayFieldFromCls = (cls: string) => { +type OverlayField = { + canonical: "map" | "mask"; + disk: "map_path" | "mask_path"; +}; +export const getOverlayFieldFromCls = (cls: string): OverlayField => { switch (cls) { case HEATMAP: return { canonical: "map", disk: "map_path" }; default: return { canonical: "mask", disk: "mask_path" }; } };app/packages/looker/src/worker/pooled-fetch.ts (1)
3-4: Consider making MAX_CONCURRENT_REQUESTS configurableThe constant value of 100 is noted as arbitrary. Consider making this configurable through environment variables or configuration files to allow tuning based on system capabilities and requirements.
-// note: arbitrary number that seems to work well -const MAX_CONCURRENT_REQUESTS = 100; +// Maximum number of concurrent requests allowed +const MAX_CONCURRENT_REQUESTS = process.env.MAX_CONCURRENT_REQUESTS + ? parseInt(process.env.MAX_CONCURRENT_REQUESTS, 10) + : 100;app/packages/looker/src/worker/decorated-fetch.ts (1)
Line range hint
26-28: Enhance error messages with request detailsConsider adding more context to error messages to aid debugging:
- throw new NonRetryableError(`Non-retryable HTTP error: ${response.status}`); + throw new NonRetryableError(`Non-retryable HTTP error ${response.status} for URL: ${url}`); - throw new Error( - "Max retries for fetch reached (linear backoff), error: " + e - ); + throw new Error( + `Max retries (${retries}) reached for URL: ${url}, last error: ${e}` + );Also applies to: 41-44
app/packages/looker/src/worker/decorated-fetch.test.ts (3)
18-18: Consider adding type for the empty options objectWhile the empty object works, explicitly typing it as
RequestInitwould improve type safety and documentation.-expect(global.fetch).toHaveBeenCalledWith("http://fiftyone.ai", {}); +expect(global.fetch).toHaveBeenCalledWith("http://fiftyone.ai", {} as RequestInit);
49-50: Consider testing with specific optionsThe test only verifies behavior with empty options. Consider adding test cases with actual fetch options to ensure they're properly passed through.
// Example additional test case: it("should properly pass through fetch options", async () => { const mockResponse = new Response("Success", { status: 200 }); global.fetch = vi.fn().mockResolvedValue(mockResponse); const options: RequestInit = { headers: { 'Content-Type': 'application/json' }, method: 'POST' }; await fetchWithLinearBackoff("http://fiftyone.ai", options); expect(global.fetch).toHaveBeenCalledWith( "http://fiftyone.ai", expect.objectContaining(options) ); });
76-81: Consider improving test readabilityThe multi-line function call could be more concise while maintaining readability.
- const fetchPromise = fetchWithLinearBackoff( - "http://fiftyone.ai", - {}, - 5, - 10 - ); + const fetchPromise = fetchWithLinearBackoff("http://fiftyone.ai", {}, 5, 10);app/packages/looker/src/overlays/base.ts (1)
43-46: Well-structured type definition supporting the performance improvement initiative.The
LabelMasktype elegantly supports the transition from raw image buffers toImageBitmap, making both properties optional for backward compatibility during the migration.Consider adding JSDoc comments to document:
- The purpose of this type in the context of worker-based image processing
- The relationship between
bitmapanddataproperties- When to expect each property to be present/absent
app/packages/looker/src/overlays/segmentation.ts (1)
80-87: Consider extracting alpha handling to a reusable function.The alpha handling logic could be extracted to a reusable function to maintain consistency across different overlay types.
+const drawWithAlpha = ( + ctx: CanvasRenderingContext2D, + alpha: number, + drawFn: () => void +) => { + const tmp = ctx.globalAlpha; + ctx.globalAlpha = alpha; + drawFn(); + ctx.globalAlpha = tmp; +}; if (this.label.mask?.bitmap) { const [tlx, tly] = t(state, 0, 0); const [brx, bry] = t(state, 1, 1); - const tmp = ctx.globalAlpha; - ctx.globalAlpha = state.options.alpha; - ctx.drawImage(this.label.mask.bitmap, tlx, tly, brx - tlx, bry - tly); - ctx.globalAlpha = tmp; + drawWithAlpha(ctx, state.options.alpha, () => { + ctx.drawImage(this.label.mask.bitmap, tlx, tly, brx - tlx, bry - tly); + }); }app/packages/looker/src/worker/disk-overlay-decoder.test.ts (3)
15-19: Remove unused mock constant.The
DETECTIONconstant is mocked but never used in the test suite. Consider removing it to maintain clean and minimal mocks.vi.mock("@fiftyone/utilities", () => ({ - DETECTION: "Detection", DETECTIONS: "Detections", HEATMAP: "Heatmap", }));
36-216: Consider adding more test cases for comprehensive coverage.The test suite is well-structured and covers the main scenarios. However, consider adding the following test cases to improve coverage:
- Test concurrent processing by verifying the behavior when multiple
maskPathDecodingPromisesare processed simultaneously- Test memory cleanup by verifying that resources (e.g., ArrayBuffers) are properly disposed
- Add edge cases for shape validation (e.g., empty shape, invalid dimensions)
Here's a sample test case for concurrent processing:
it("should handle concurrent processing of multiple overlays", async () => { const maskPathDecodingPromises: Promise<void>[] = []; const labels = Array(3).fill(null).map((_, i) => ({ mask_path: `/path/to/mask${i}`, mask: null as MaskUnion })); // Setup mocks for concurrent requests vi.mocked(getSampleSrc).mockImplementation(path => `http://example.com${path}`); vi.mocked(fetchWithLinearBackoff).mockImplementation(() => Promise.resolve({ blob: () => Promise.resolve(new Blob(['mock data'])) } as Response) ); // Process labels concurrently await Promise.all(labels.map(label => decodeOverlayOnDisk( 'field', label, COLORING, CUSTOMIZE_COLOR_SETTING, COLOR_SCALE, SOURCES, 'Segmentation', maskPathDecodingPromises ) )); await Promise.all(maskPathDecodingPromises); // Verify all labels were processed labels.forEach(label => { expect(label.mask).toBeDefined(); expect(label.mask.image).toBeInstanceOf(ArrayBuffer); }); });
187-215: Enhance error handling test coverage.The error handling test case could be more comprehensive. Consider:
- Testing specific error types (network errors, timeout errors, etc.)
- Validating error messages
- Testing retry behavior
Here's an example enhancement:
it("should handle different types of fetch errors", async () => { const label = { mask_path: "/path/to/mask", mask: null as MaskUnion }; const maskPathDecodingPromises: Promise<void>[] = []; // Test network error vi.mocked(fetchWithLinearBackoff).mockRejectedValueOnce( new TypeError("Failed to fetch") ); await decodeOverlayOnDisk( "field", label, COLORING, CUSTOMIZE_COLOR_SETTING, COLOR_SCALE, SOURCES, "Segmentation", maskPathDecodingPromises ); expect(label.mask).toBeNull(); // Test timeout error vi.mocked(fetchWithLinearBackoff).mockRejectedValueOnce( new Error("Timeout") ); await decodeOverlayOnDisk( "field", label, COLORING, CUSTOMIZE_COLOR_SETTING, COLOR_SCALE, SOURCES, "Segmentation", maskPathDecodingPromises ); expect(label.mask).toBeNull(); // Verify retry behavior expect(fetchWithLinearBackoff).toHaveBeenCalledTimes(2); });app/packages/looker/src/lookers/abstract.ts (1)
Line range hint
601-644: Add timeout and error handling for worker communicationThe worker communication lacks timeout handling and error recovery mechanisms. Long-running or failed worker operations could leave the system in an inconsistent state.
Consider implementing timeout and error handling:
private loadSample(sample: Sample) { const messageUUID = uuid(); + const timeout = setTimeout(() => { + labelsWorker.removeEventListener("message", listener); + this.updater({ + error: new Error("Worker timeout: Failed to process sample"), + disabled: true, + reloading: false + }); + }, 30000); // 30 second timeout const labelsWorker = getLabelsWorker(); const listener = ({ data: { sample, coloring, uuid } }) => { if (uuid === messageUUID) { + clearTimeout(timeout); this.sample = sample; this.state.options.coloring = coloring; this.loadOverlays(sample); this.updater({ overlaysPrepared: true, disabled: false, reloading: false, }); labelsWorker.removeEventListener("message", listener); } }; + labelsWorker.addEventListener("error", (error) => { + clearTimeout(timeout); + labelsWorker.removeEventListener("message", listener); + this.updater({ + error: new Error(`Worker error: ${error.message}`), + disabled: true, + reloading: false + }); + }); labelsWorker.addEventListener("message", listener);app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
21-21: Use a specific type instead of 'Record<string, any>'The
labelparameter is typed asRecord<string, any>, which is too generic. Defining a specific interface forlabelwould enhance type safety and code maintainability.app/packages/looker/src/overlays/heatmap.ts (1)
Line range hint
100-150: Add null checks to prevent potential runtime errorsIn the methods
getIndex,getMapCoordinates, andgetTarget,this.label.mapandthis.targetsmay be undefined if the constructor returns early due to a missinglabel.map. Accessing their properties without null checks could lead to runtime errors.Apply the following diffs to add null checks:
In
getIndex:private getIndex(state: Readonly<State>): number { + if (!this.label.map) { + return -1; + } const [sx, sy] = this.getMapCoordinates(state); if (sx < 0 || sy < 0) { return -1; } return this.label.map.data.shape[1] * sy + sx; }In
getMapCoordinates:private getMapCoordinates({ pixelCoordinates: [x, y], dimensions: [mw, mh], }: Readonly<State>): Coordinates { + if (!this.label.map) { + return [-1, -1]; + } const [h, w] = this.label.map.data.shape; const sx = Math.floor(x * (w / mw)); const sy = Math.floor(y * (h / mh)); return [sx, sy]; }In
getTarget:private getTarget(state: Readonly<State>): number { const index = this.getIndex(state); if (index < 0) { return null; } + if (!this.label.map || !this.targets) { + return null; + } if (this.label.map.data.channels > 1) { return this.targets[index * this.label.map.data.channels]; } return this.targets[index]; }app/packages/looker/src/worker/index.ts (3)
258-258: Consider using thedeleteoperator to remove theimagepropertyInstead of setting
label[overlayField].imagetonull, you could delete the property to potentially free up memory more effectively:- label[overlayField].image = null; + delete label[overlayField].image;This change explicitly removes the property from the object.
261-267: Simplify Promise creation incollectBitmapPromisesYou can simplify the Promise handling by directly returning the
createImageBitmappromise:-bitmapPromises.push( - new Promise((resolve) => { - createImageBitmap(imageData).then((imageBitmap) => { - label[overlayField].bitmap = imageBitmap; - resolve(imageBitmap); - }); - }) -); +bitmapPromises.push( + createImageBitmap(imageData).then((imageBitmap) => { + label[overlayField].bitmap = imageBitmap; + return imageBitmap; + }) +);This refactor reduces unnecessary nesting and makes the code more concise.
332-332: Simplify conditional check with optional chainingConsider simplifying the conditional statement using optional chaining:
-if (sample.frames && sample.frames.length) { +if (sample.frames?.length) {This makes the code cleaner and handles cases where
sample.framesisundefinedornull.🧰 Tools
🪛 Biome (1.9.4)
[error] 332-332: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
app/packages/looker/src/lookers/abstract.ts(1 hunks)app/packages/looker/src/overlays/base.ts(2 hunks)app/packages/looker/src/overlays/detection.ts(3 hunks)app/packages/looker/src/overlays/heatmap.ts(3 hunks)app/packages/looker/src/overlays/segmentation.ts(5 hunks)app/packages/looker/src/worker/decorated-fetch.test.ts(5 hunks)app/packages/looker/src/worker/decorated-fetch.ts(1 hunks)app/packages/looker/src/worker/deserializer.ts(0 hunks)app/packages/looker/src/worker/disk-overlay-decoder.test.ts(1 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(1 hunks)app/packages/looker/src/worker/index.ts(8 hunks)app/packages/looker/src/worker/pooled-fetch.ts(1 hunks)app/packages/looker/src/worker/shared.ts(2 hunks)
💤 Files with no reviewable changes (1)
- app/packages/looker/src/worker/deserializer.ts
🧰 Additional context used
📓 Path-based instructions (12)
app/packages/looker/src/lookers/abstract.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/overlays/base.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/overlays/detection.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/overlays/heatmap.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/overlays/segmentation.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/decorated-fetch.test.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/decorated-fetch.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/disk-overlay-decoder.test.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/index.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/pooled-fetch.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/shared.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🪛 Biome (1.9.4)
app/packages/looker/src/worker/index.ts
[error] 332-332: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (23)
app/packages/looker/src/worker/shared.ts (2)
1-1: LGTM!
The named import follows TypeScript best practices and supports tree-shaking.
14-21: Verify worker thread integration.
Let's ensure this utility function is properly integrated with the worker thread's ImageBitmap creation process.
✅ Verification successful
Worker thread integration is properly implemented
The utility function getOverlayFieldFromCls is correctly integrated into the worker thread's image processing pipeline:
- Used in
disk-overlay-decoder.tsto determine overlay field names for disk-based overlays - Integrated in
index.tswhere it's used in conjunction withcreateImageBitmapto process overlay data - Properly handles both heatmap and mask cases, feeding into the ImageBitmap creation process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of getOverlayFieldFromCls in worker-related files
rg -t ts "getOverlayFieldFromCls" --glob "app/packages/looker/src/worker/**"
# Verify the relationship with ImageBitmap creation
ast-grep --pattern 'createImageBitmap($$$)'
Length of output: 834
Script:
#!/bin/bash
# Let's check the context around the usage of getOverlayFieldFromCls
rg -t ts -B 5 -A 5 "getOverlayFieldFromCls" --glob "app/packages/looker/src/worker/**"
# Also check how ImageBitmap is used in relation to overlays
rg -t ts -B 5 -A 5 "createImageBitmap" --glob "app/packages/looker/src/worker/**"
Length of output: 5998
app/packages/looker/src/worker/decorated-fetch.ts (2)
15-15: LGTM! Well-structured parameter addition
The new opts parameter is well-typed using RequestInit and maintains backward compatibility with its default empty object value.
21-21: Consider cloning the opts object for each retry attempt
The current implementation reuses the same opts object across retry attempts. This could cause issues if opts contains a request body stream, as streams can only be read once. Consider cloning the opts object for each retry attempt.
Here's a suggested improvement:
- const response = await fetch(url, opts);
+ const response = await fetch(url, { ...opts });Let's verify if the codebase uses request body streams:
app/packages/looker/src/worker/decorated-fetch.test.ts (3)
38-38: LGTM! Test case properly validates retry behavior
The test case correctly verifies the error handling with the new options parameter.
60-61: LGTM! Non-retryable error handling is correct
The test properly verifies that 4xx errors are immediately thrown without retrying.
Line range hint 1-96: Verify integration with worker thread changes
While the test changes look good, we should verify how this relates to the worker thread improvements mentioned in the PR objectives.
app/packages/looker/src/overlays/base.ts (1)
6-6: LGTM!
Clean import statement following TypeScript conventions.
app/packages/looker/src/overlays/segmentation.ts (3)
5-5: LGTM: Import changes align with new functionality.
The added imports support the new bitmap-based mask handling and size estimation features.
Also applies to: 11-11
52-52: LGTM: Constructor simplified with bitmap-based approach.
The removal of canvas and imageData initialization aligns with the performance improvement goals by using ImageBitmap.
20-20: Verify the impact of the LabelMask type change.
The change from OverlayMask to LabelMask type might affect consumers of this interface.
✅ Verification successful
Type change from OverlayMask to LabelMask appears safe
The change appears to be part of a coordinated refactoring:
LabelMaskis a shared type used across multiple overlay types (detection, heatmap, segmentation)- The actual mask data handling in worker files still uses
OverlayMaskfor the underlying implementation LabelMaskappears to be an interface type for label definitions whileOverlayMaskhandles the data implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct usage of OverlayMask type with segmentation
rg -t typescript "OverlayMask.*segmentation"
# Search for implementations of SegmentationLabel interface
ast-grep --pattern 'interface $_ extends SegmentationLabel'
Length of output: 143
Script:
#!/bin/bash
# Search for OverlayMask usage
rg "OverlayMask"
# Search for SegmentationLabel interface implementations and usage
rg "SegmentationLabel"
# Search for LabelMask usage
rg "LabelMask"
# Get the content of the file to understand the context
cat app/packages/looker/src/overlays/segmentation.ts
Length of output: 9289
app/packages/looker/src/overlays/detection.ts (4)
9-15: LGTM: Import changes align with the new LabelMask type
The imports are well-organized and properly introduce the new LabelMask type needed for the performance improvements.
19-19: LGTM: Interface update supports performance improvements
The simplified mask property type using LabelMask aligns with the PR's objective to improve rendering performance by transitioning to ImageBitmap.
34-34: LGTM: Property addition supports new bitmap-based rendering
The imageBitmap property is properly typed and initialized, supporting the transition to bitmap-based rendering.
147-157: LGTM: Efficient bitmap-based rendering implementation
The implementation correctly uses ImageBitmap for rendering, with image smoothing disabled for better performance.
app/packages/looker/src/worker/disk-overlay-decoder.test.ts (1)
34-34: LGTM! Clear and well-defined type.
The MaskUnion type effectively combines the necessary types for testing purposes.
app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
85-85: Verify the order of dimensions in 'overlayMask.shape'
Ensure that overlayMask.shape returns [height, width] as expected. If it returns [width, height], the variables overlayHeight and overlayWidth should be assigned accordingly.
app/packages/looker/src/overlays/heatmap.ts (5)
9-9: Importing 'sizeBytesEstimate' for size estimation
The import statement correctly adds sizeBytesEstimate from @fiftyone/utilities for calculating the memory footprint.
18-18: Importing 'LabelMask' for label definitions
The LabelMask is appropriately imported from ./base for use in the HeatmapOverlay class.
27-27: Updated 'map' property type to 'LabelMask'
The map property in the HeatmapLabel interface is correctly updated to use LabelMask, aligning with the refactoring to simplify mask structures across overlays.
80-85: Simplified drawing logic using 'ImageBitmap'
The draw method now efficiently utilizes this.label.map?.bitmap to render the heatmap overlay directly, enhancing rendering performance.
205-207: Added getSizeBytes method for memory footprint estimation
The getSizeBytes method correctly uses sizeBytesEstimate to provide the estimated size of the heatmap label, aiding in resource management.
app/packages/looker/src/worker/index.ts (1)
97-104: Well-documented function steps in processLabels
The comprehensive comments outlining the steps in processLabels enhance readability and understanding of the processing workflow.
sashankaryal
force-pushed
the
improv/image-bitmaps
branch
from
3165f1e to
a56869c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
app/packages/looker/src/overlays/heatmap.ts (2)
80-87: LGTM: Efficient bitmap-based drawing implementation.The implementation correctly uses
drawImagewith the bitmap, maintaining alpha transparency. Consider caching theglobalAlpharestoration using a try-finally block for better error handling.Consider this more robust implementation:
if (this.label.map?.bitmap) { const [tlx, tly] = t(state, 0, 0); const [brx, bry] = t(state, 1, 1); const tmp = ctx.globalAlpha; - ctx.globalAlpha = state.options.alpha; - ctx.drawImage(this.label.map.bitmap, tlx, tly, brx - tlx, bry - tly); - ctx.globalAlpha = tmp; + try { + ctx.globalAlpha = state.options.alpha; + ctx.drawImage(this.label.map.bitmap, tlx, tly, brx - tlx, bry - tly); + } finally { + ctx.globalAlpha = tmp; + } }
209-213: LGTM: Proper resource cleanup implementation.The cleanup method correctly releases the bitmap resource, but the optional chaining can be simplified.
Consider this more concise version:
public cleanup(): void { - if (this.label.map?.bitmap) { - this.label.map?.bitmap.close(); - } + this.label.map?.bitmap?.close(); }app/packages/looker/src/overlays/detection.ts (1)
146-156: Add error handling for bitmap accessWhile the changes improve performance by using ImageBitmap directly, consider adding error handling for potential bitmap access issues.
private drawMask(ctx: CanvasRenderingContext2D, state: Readonly<State>) { if (!this.label.mask?.bitmap) { + console.warn('Mask bitmap not available for drawing'); return; } + try { const [tlx, tly, w, h] = this.label.bounding_box; const [x, y] = t(state, tlx, tly); const tmp = ctx.globalAlpha; ctx.globalAlpha = state.options.alpha; ctx.imageSmoothingEnabled = false; ctx.drawImage( this.label.mask.bitmap, x, y, w * state.canvasBBox[2], h * state.canvasBBox[3] ); ctx.globalAlpha = tmp; + } catch (error) { + console.error('Error drawing mask bitmap:', error); + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
app/packages/looker/src/lookers/abstract.ts(1 hunks)app/packages/looker/src/overlays/base.ts(3 hunks)app/packages/looker/src/overlays/detection.ts(3 hunks)app/packages/looker/src/overlays/heatmap.ts(3 hunks)app/packages/looker/src/overlays/segmentation.ts(5 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(1 hunks)app/packages/looker/src/worker/pooled-fetch.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/packages/looker/src/lookers/abstract.ts
- app/packages/looker/src/overlays/base.ts
- app/packages/looker/src/worker/disk-overlay-decoder.ts
- app/packages/looker/src/worker/pooled-fetch.ts
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/looker/src/overlays/detection.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/overlays/heatmap.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/overlays/segmentation.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (10)
app/packages/looker/src/overlays/heatmap.ts (3)
9-9: LGTM: Import changes align with new functionality.
The new imports support the bitmap-based implementation and size estimation features.
Also applies to: 18-18
205-207: LGTM: Memory estimation method added.
The getSizeBytes implementation provides useful memory usage information.
27-27: LGTM: Interface update supports bitmap-based rendering.
The change from HeatMap to LabelMask aligns with the new bitmap-based rendering approach.
Let's verify that all usages of the old HeatMap interface have been updated:
✅ Verification successful
Interface change is safe and properly implemented
The search results confirm that:
- The old
HeatMapinterface is completely removed from the codebase LabelMaskis properly defined inbase.tsand consistently used across different overlay types (heatmap, detection, segmentation)- All relevant files have been updated to use the new
LabelMasktype
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old HeatMap interface
rg "HeatMap[^L]" --type ts
Length of output: 28
Script:
#!/bin/bash
# Let's try a more comprehensive search for HeatMap
rg "HeatMap" --type ts -A 3
# Also search for any files containing both HeatMap and LabelMask
rg "LabelMask" --type ts -A 3
Length of output: 2597
app/packages/looker/src/overlays/segmentation.ts (5)
5-5: LGTM: Interface changes align with performance improvements
The simplified SegmentationLabel interface using LabelMask type supports the transition to ImageBitmap-based rendering.
Also applies to: 11-11, 20-20
52-52: LGTM: Constructor simplified for bitmap-based rendering
The removal of canvas initialization code aligns with the performance improvement goals while maintaining necessary target data handling.
260-262: Size estimation doesn't include bitmap memory
This is a known limitation where sizeBytesEstimate doesn't account for ImageBitmap memory usage.
80-87: LGTM: Efficient bitmap-based drawing implementation
The direct use of drawImage with ImageBitmap eliminates unnecessary canvas operations while maintaining proper alpha channel handling.
Let's verify the performance impact:
264-268: LGTM: Proper bitmap resource cleanup
The cleanup method correctly releases bitmap resources, which is essential for preventing memory leaks.
Let's verify cleanup is called appropriately:
app/packages/looker/src/overlays/detection.ts (2)
9-15: LGTM: Import changes align with the new ImageBitmap approach
The addition of LabelMask import supports the refactoring to use ImageBitmap for improved rendering performance.
19-19: LGTM: Interface change simplifies mask handling
The simplified mask property type aligns with the performance improvements by using LabelMask type for ImageBitmap handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/packages/looker/src/overlays/detection.ts (1)
Line range hint
146-161: Improve bitmap access patternThe current implementation has a few areas for improvement:
- The optional chaining on bitmap access could be more concise
- Consider adding error handling for drawImage operation
- if (!this.label.mask?.bitmap) { + const bitmap = this.label.mask?.bitmap; + if (!bitmap) { return; } const [tlx, tly, w, h] = this.label.bounding_box; const [x, y] = t(state, tlx, tly); const tmp = ctx.globalAlpha; ctx.globalAlpha = state.options.alpha; ctx.imageSmoothingEnabled = false; - ctx.drawImage( - this.label.mask.bitmap, - x, - y, - w * state.canvasBBox[2], - h * state.canvasBBox[3] - ); + try { + ctx.drawImage( + bitmap, + x, + y, + w * state.canvasBBox[2], + h * state.canvasBBox[3] + ); + } catch (error) { + console.error('Error drawing bitmap:', error); + } ctx.globalAlpha = tmp;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/looker/src/overlays/detection.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/overlays/detection.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/looker/src/overlays/detection.ts (2)
264-269: 🛠️ Refactor suggestion
Improve cleanup method implementation
The cleanup method could be more robust:
- Remove redundant optional chaining
- Add error handling for bitmap closure
public cleanup(): void {
- if (this.label.mask?.bitmap) {
- this.label.mask?.bitmap.close();
- this.label.mask.bitmap = null;
+ const bitmap = this.label.mask?.bitmap;
+ if (bitmap) {
+ try {
+ bitmap.close();
+ this.label.mask.bitmap = null;
+ } catch (error) {
+ console.error('Error closing bitmap:', error);
+ }
}
}9-15: Verify the impact of DetectionLabel interface changes
The change from a complex mask object to LabelMask type is a breaking change that could affect other parts of the codebase.
Also applies to: 19-19
✅ Verification successful
The DetectionLabel interface change is safe to proceed
The codebase analysis shows that the DetectionLabel interface is used consistently across the codebase, and the mask property is already being handled as an optional field with proper type checking. The existing mask property access patterns in the codebase (.mask.data and .mask.image) align with the LabelMask type structure, indicating this is a type refinement rather than a breaking change.
Key findings:
- All mask property accesses are properly guarded and follow the expected pattern
- The interface is primarily used within the looker package's internal components
- No external dependencies were found that could be impacted by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of DetectionLabel interface
rg -A 3 "DetectionLabel" --type typescript
# Search for mask property access patterns
rg -A 2 "\.mask\.(data|image)" --type typescript
Length of output: 163
Script:
#!/bin/bash
# Search for usages of DetectionLabel interface without file type restriction
rg -A 3 "DetectionLabel"
# Search for mask property access patterns
rg -A 2 "\.mask\.(data|image)"
# Find all TypeScript files that might use DetectionLabel
fd "\.tsx?$" | xargs rg "DetectionLabel"
Length of output: 14999
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
app/packages/looker/src/worker/index.ts (4)
97-104: Consider enhancing function documentationWhile the step-by-step documentation is clear, consider adding:
@paramdescriptions for each parameter@returnsdescription explaining the tuple of bitmap promises and mask target buffers
216-231: Consider batching bitmap creation for better memory managementThe bitmap generation loop processes each label sequentially. For large datasets, this could lead to high memory usage as all ImageBitmap promises are created simultaneously.
Consider implementing a batching mechanism:
- for (const field in sample) { - let labels = sample[field]; - // ... existing code ... - for (const label of labels) { - collectBitmapPromises(label, cls, bitmapPromises); - } - } + const BATCH_SIZE = 50; // Adjust based on performance testing + const allLabels = Object.entries(sample).flatMap(([field, labels]) => { + const cls = getCls(`${prefix ? prefix : ""}${field}`, schema); + return (Array.isArray(labels) ? labels : [labels]) + .filter(label => label) + .map(label => ({ label, cls })); + }); + + for (let i = 0; i < allLabels.length; i += BATCH_SIZE) { + const batch = allLabels.slice(i, i + BATCH_SIZE); + await Promise.all( + batch.map(({ label, cls }) => + collectBitmapPromises(label, cls, bitmapPromises) + ) + ); + }
337-352: Use optional chaining and improve type safety- if (sample.frames && sample.frames.length) { + if (sample.frames?.length) { - for (const frame of sample.frames) { + for (const frame of sample.frames as Array<typeof sample>) { const [moreBitmapPromises, moreMaskTargetsBuffers] = await processLabels( frame, coloring, "frames.", sources, customizeColorSetting, colorscale, labelTagColors, selectedLabelTags, schema ); imageBitmapPromises.push(...moreBitmapPromises); maskTargetsBuffers.push(...moreMaskTargetsBuffers); } }🧰 Tools
🪛 Biome (1.9.4)
[error] 337-337: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
355-369: Consider handling Promise.all rejectionThe final Promise.all could fail if any bitmap creation fails, potentially leaving the main thread waiting.
- Promise.all(imageBitmapPromises).then((bitmaps) => { + Promise.all(imageBitmapPromises).then((bitmaps) => { postMessage( { method: "processSample", sample, coloring, uuid, customizeColorSetting, colorscale, labelTagColors, selectedLabelTags, }, // @ts-ignore bitmaps.flat().concat(maskTargetsBuffers.flat()) ); + }).catch((error) => { + postMessage({ + method: "processSample", + error: { + message: "Failed to process bitmaps", + details: error.message + } + }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/looker/src/worker/disk-overlay-decoder.test.ts(1 hunks)app/packages/looker/src/worker/index.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/looker/src/worker/disk-overlay-decoder.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/worker/index.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🪛 Biome (1.9.4)
app/packages/looker/src/worker/index.ts
[error] 337-337: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
app/packages/looker/src/worker/index.ts (1)
116-119: LGTM! Clear separation of concerns with promise arrays
The separation into different promise arrays for mask decoding, painting, and bitmap generation improves code clarity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/packages/looker/src/worker/index.ts (4)
Line range hint
97-116: Consider using type aliases for better readability.The return type
Promise<[Promise<ImageBitmap[]>[], ArrayBuffer[]]>is complex and could be more readable with type aliases.+type BitmapPromises = Promise<ImageBitmap[]>[]; +type MaskBuffers = ArrayBuffer[]; +type ProcessLabelsResult = Promise<[BitmapPromises, MaskBuffers]>; -): Promise<[Promise<ImageBitmap[]>[], ArrayBuffer[]]> => { +): ProcessLabelsResult => {
Line range hint
122-247: Consider extracting phase-specific processing into separate functions.The three processing phases (mask deserialization, overlay painting, and bitmap generation) each contain similar loop structures. Extracting these into separate functions would improve maintainability and reduce code duplication.
+const processMasks = async (sample, prefix, schema, ...) => { + const maskPathDecodingPromises = []; + for (const field in sample) { + // mask deserialization logic + } + return maskPathDecodingPromises; +}; +const processOverlays = async (sample, prefix, schema, ...) => { + const painterPromises = []; + for (const field in sample) { + // overlay painting logic + } + return painterPromises; +}; +const processBitmaps = (sample, prefix, schema, ...) => { + const bitmapPromises = []; + for (const field in sample) { + // bitmap generation logic + } + return bitmapPromises; +};
332-387: Consider implementing batch processing for large frame sets.Processing all frames at once could lead to high memory usage with large frame sets. Consider implementing batch processing or a memory-aware processing strategy.
const processBatch = async (frames, batchSize = 10) => { for (let i = 0; i < frames.length; i += batchSize) { const batch = frames.slice(i, i + batchSize); // Process batch await new Promise(resolve => setTimeout(resolve, 0)); // Yield to main thread } };
Line range hint
542-552: Fix incorrect array destructuring in Promise.all handler.The destructuring
[bitmaps, buffers]assumes processLabels returns a single pair, but it actually returns an array of pairs (one for each frame).- ).then(([bitmaps, buffers]) => { + ).then((results) => { + const bitmaps = results.map(([b, _]) => b).flat(); + const buffers = results.map(([_, b]) => b).flat(); const transferables = [...bitmaps.flat(), ...buffers.flat()];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/looker/src/worker/index.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/worker/index.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (1)
app/packages/looker/src/worker/index.ts (1)
252-289: Previous review comment about error handling is still applicable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/packages/looker/src/worker/index.ts (1)
545-546: Consider using explicit array destructuringThe current destructuring might be fragile if the return type of
processLabelschanges in the future. Consider using a more explicit approach.Apply this diff to make the destructuring more robust:
- ).then(([bitmaps, buffers]) => { + ).then((results) => { + const [bitmaps = [], buffers = []] = results;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/looker/src/worker/disk-overlay-decoder.ts(1 hunks)app/packages/looker/src/worker/index.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/looker/src/worker/disk-overlay-decoder.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/worker/index.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🪛 Biome (1.9.4)
app/packages/looker/src/worker/index.ts
[error] 211-211: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
🔇 Additional comments (1)
app/packages/looker/src/worker/index.ts (1)
97-104: Implementation aligns with performance objectives
The refactoring successfully implements the creation of ImageBitmaps in the worker thread, which should improve rendering performance by reducing main thread workload. The code is well-documented with clear processing steps.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
app/packages/looker/src/worker/disk-overlay-decoder.ts (6)
10-13: Add JSDoc documentation for IntermediateMask type.Consider adding documentation to explain the purpose and usage of this type, particularly the relationship between
dataandimageproperties.+/** + * Represents an intermediate state of mask processing + * @property data - The decoded overlay mask data + * @property image - The raw image buffer for pixel manipulation + */ export type IntermediateMask = { data: OverlayMask; image: ArrayBuffer; };
19-29: Consider using a more specific type for the label parameter.Instead of using
Record<string, any>, consider creating a specific type or interface that describes the expected shape of the label object. This would improve type safety and code maintainability.+type OverlayLabel = { + detections?: OverlayLabel[]; + [overlayField: string]: unknown; +}; export const decodeOverlayOnDisk = async ( field: string, - label: Record<string, any>, + label: OverlayLabel, coloring: Coloring, customizeColorSetting: CustomizeColor[], colorscale: Colorscale, sources: { [path: string]: string }, cls: string, maskPathDecodingPromises: Promise<void>[] = [], maskTargetsBuffers: ArrayBuffer[] = [] )
57-67: Use optional chaining for safer property access.The current nested property access could throw if any intermediate property is undefined.
if ( - label[overlayField] && - label[overlayField].bitmap && - !label[overlayField].image + label[overlayField]?.bitmap && + !label[overlayField]?.image ) { const height = label[overlayField].bitmap.height; const width = label[overlayField].bitmap.width; label[overlayField].image = new ArrayBuffer(height * width * 4); label[overlayField].bitmap.close(); label[overlayField].bitmap = null; }🧰 Tools
🪛 Biome (1.9.4)
[error] 58-59: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
72-83: Add URL validation and sanitization.Consider adding validation for the URL construction to handle edge cases and prevent potential security issues.
+const isValidUrl = (url: string): boolean => { + try { + new URL(url); + return true; + } catch { + return false; + } +}; // convert absolute file path to a URL that we can "fetch" from const overlayImageUrl = getSampleSrc( sources[`${field}.${overlayPathField}`] || label[overlayPathField] ); +if (!isValidUrl(overlayImageUrl)) { + console.error('Invalid URL:', overlayImageUrl); + return; +}
85-106: Consider using custom error types for better error handling.The current error handling could be more specific to help with debugging and user feedback.
+class OverlayFetchError extends Error { + constructor(message: string, public url: string) { + super(message); + this.name = 'OverlayFetchError'; + } +} +class OverlayDecodeError extends Error { + constructor(message: string) { + super(message); + this.name = 'OverlayDecodeError'; + } +} try { const overlayImageFetchResponse = await enqueueFetch({ url: baseUrl, options: { priority: "low" }, }); overlayImageBlob = await overlayImageFetchResponse.blob(); } catch (e) { - console.error(e); + console.error(new OverlayFetchError(e.message, baseUrl)); return; }
107-121: Add validation for overlay dimensions.Consider adding checks for valid dimensions to prevent potential issues with buffer allocation.
const [overlayHeight, overlayWidth] = overlayMask.shape; + +if (overlayHeight <= 0 || overlayWidth <= 0) { + console.error('Invalid overlay dimensions:', { overlayHeight, overlayWidth }); + return; +} + +// Prevent excessive memory allocation +const MAX_DIMENSION = 16384; // Example limit +if (overlayHeight > MAX_DIMENSION || overlayWidth > MAX_DIMENSION) { + console.error('Overlay dimensions exceed limit:', { overlayHeight, overlayWidth }); + return; +} label[overlayField] = { data: overlayMask, image: new ArrayBuffer(overlayWidth * overlayHeight * 4), } as IntermediateMask;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/looker/src/worker/disk-overlay-decoder.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🪛 Biome (1.9.4)
app/packages/looker/src/worker/disk-overlay-decoder.ts
[error] 58-59: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
sashankaryal
force-pushed
the
improv/image-bitmaps
branch
from
bb1f805 to
f65aa4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
app/packages/looker/src/worker/disk-overlay-decoder.ts (4)
15-29: Enhance function documentation with parameter descriptions.While the function's purpose is well-documented, consider adding parameter descriptions to improve maintainability.
Add parameter descriptions to the JSDoc:
/** * Some label types (example: segmentation, heatmap) can have their overlay data stored on-disk, * we want to impute the relevant mask property of these labels from what's stored in the disk + * + * @param field - The field name containing the overlay data + * @param label - The label object containing overlay information + * @param coloring - Coloring configuration + * @param customizeColorSetting - Custom color settings array + * @param colorscale - Color scale configuration + * @param sources - Mapping of paths to their source URLs + * @param cls - The class type (e.g., DETECTION, DETECTIONS) + * @param maskPathDecodingPromises - Array to collect decoding promises + * @param maskTargetsBuffers - Array to collect target buffers for transfer */
55-70: Use optional chaining for safer property access.The nested property access could be simplified using optional chaining.
Apply this diff:
- if ( - label[overlayField] && - label[overlayField].bitmap && - !label[overlayField].image - ) { + if (label[overlayField]?.bitmap && !label[overlayField]?.image) {🧰 Tools
🪛 Biome (1.9.4)
[error] 58-59: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
62-67: Extract bitmap cleanup logic into a helper function.The bitmap cleanup and buffer initialization logic could be more maintainable as a separate function.
Consider extracting the logic:
function cleanupBitmap(overlay: any, height: number, width: number) { overlay.image = new ArrayBuffer(height * width * 4); overlay.bitmap.close(); overlay.bitmap = null; }
118-120: Clarify the buffer transfer comment.The current comment could be more precise about why the image buffer isn't transferred.
Update the comment to be more explicit:
- // no need to transfer image's buffer - //since we'll be constructing ImageBitmap and transfering that + // Skip transferring the image buffer as we'll be constructing and + // transferring an ImageBitmap instead, which is more efficient for renderingapp/packages/looker/src/worker/index.ts (3)
226-247: Consider enhancing error handling in bitmap generation loopThe bitmap generation loop could benefit from error handling to ensure robustness.
Consider wrapping the loop in a try-catch block:
// bitmap generation loop + try { for (const field in sample) { let labels = sample[field]; if (!Array.isArray(labels)) { labels = [labels]; } const cls = getCls(`${prefix ? prefix : ""}${field}`, schema); if (!cls) { continue; } for (const label of labels) { if (!label) { continue; } collectBitmapPromises(label, cls, bitmapPromises); } } + } catch (error) { + console.error('Error in bitmap generation:', error); + // Consider how to handle failed bitmap generation + }
332-333: Consider adding explicit type definitionsThe return type inference could be made more explicit for better maintainability.
Consider adding type definitions:
- const imageBitmapPromises: Promise<ImageBitmap[]>[] = []; - let maskTargetsBuffers: ArrayBuffer[] = []; + type BitmapPromises = Promise<ImageBitmap[]>[]; + type MaskBuffers = ArrayBuffer[]; + const imageBitmapPromises: BitmapPromises = []; + let maskTargetsBuffers: MaskBuffers = [];
Line range hint
542-552: Consider adding type assertion for transferablesThe transferables array could benefit from explicit typing.
Consider adding type assertion:
- const transferables = [...bitmaps.flat(), ...buffers.flat()]; + const transferables = [...bitmaps.flat(), ...buffers.flat()] as Transferable[];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/looker/src/worker/disk-overlay-decoder.ts(1 hunks)app/packages/looker/src/worker/index.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/index.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🪛 Biome (1.9.4)
app/packages/looker/src/worker/disk-overlay-decoder.ts
[error] 58-59: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
app/packages/looker/src/worker/disk-overlay-decoder.ts (2)
1-13: LGTM: Well-organized imports and clear type definition.
The imports are properly organized, and the IntermediateMask type clearly defines the structure needed for intermediate processing of mask data.
72-96: LGTM: Robust URL handling with proper error management.
The code properly handles URL construction, query parameters, and fetch errors with appropriate priority settings.
app/packages/looker/src/worker/index.ts (3)
31-33: LGTM: Import changes align with performance improvements
The new imports support the bitmap-based rendering approach and maintain good code organization.
Line range hint 97-250: LGTM: Well-structured implementation with clear processing phases
The three-phase approach (mask deserialization, overlay painting, bitmap generation) is well-documented and efficiently implemented. The memory management strategy of clearing raw image data after bitmap creation is particularly good.
252-289: Previous review comment about error handling is still applicable
The existing review comment suggesting error handling and type safety improvements for the bitmap creation logic is still valid and should be addressed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/packages/looker/src/worker/index.ts (3)
Line range hint
97-116: Consider adding TypeScript type definitions for the return tupleThe documentation clearly explains the processing steps. However, the return type could be more explicit with proper TypeScript types.
Consider adding a type definition:
type ProcessLabelsResult = [Promise<ImageBitmap[]>[], ArrayBuffer[]]; const processLabels = async ( // ... existing parameters ... ): Promise<ProcessLabelsResult> => {
Line range hint
122-184: Consider extracting label processing loops into separate functionsThe label processing logic is well-structured but could be more maintainable if broken down into smaller, focused functions.
Consider refactoring into separate functions:
const processDenseLabels = async ( field: string, label: any, prefix: string, cls: string, // ... other parameters ... ) => { if (DENSE_LABELS.has(cls)) { return decodeOverlayOnDisk( // ... parameters ... ); } }; const processEmbeddedDocuments = async ( field: string, label: any, prefix: string, cls: string, // ... other parameters ... ) => { if ([EMBEDDED_DOCUMENT, DYNAMIC_EMBEDDED_DOCUMENT].includes(cls)) { return processLabels( // ... parameters ... ); } };
544-560: Optimize array operations for better performanceThe current implementation chains multiple array operations which could impact performance with large datasets.
Consider combining the operations:
const [allBuffers, allBitmapsPromises] = allLabelsResults.reduce( (acc, [bitmaps, buffers]) => { acc[0].push(...buffers); acc[1].push(...bitmaps); return acc; }, [[], []] as [ArrayBuffer[], Promise<ImageBitmap[]>[]] );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/looker/src/worker/index.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/worker/index.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (1)
app/packages/looker/src/worker/index.ts (1)
252-289: Previous review comment about error handling is still applicable
sashankaryal
force-pushed
the
improv/image-bitmaps
branch
from
589a122 to
7d5d7c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/packages/looker/src/lookers/frame-reader.ts (1)
55-63: Add error handling for cleanup operations.While the cleanup implementation is good, consider adding error handling to prevent failures during cache disposal.
Consider this safer implementation:
dispose: (frame, key) => { const overlays = frame.overlays; - for (let i = 0; i < overlays.length; i++) { - overlays[i].cleanup?.(); - } + overlays.forEach(overlay => { + try { + overlay.cleanup?.(); + } catch (error) { + console.error('Failed to cleanup overlay:', error); + } + }); removeFrame(key); },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/looker/src/lookers/frame-reader.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/lookers/frame-reader.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/looker/src/lookers/frame-reader.ts (2)
Line range hint 64-69: LGTM: Cache configuration is well-tuned.
The cache configuration is appropriate with:
noDisposeOnSet: truepreventing unnecessary cleanup- Proper size calculation for memory management
55-63: Verify cleanup coverage across disposal scenarios.
The cleanup implementation looks good, but we should verify it's called in all necessary scenarios (cache eviction, clear, terminate).
Let's verify the cleanup coverage:
✅ Verification successful
Let me analyze the cleanup coverage based on the search results.
The first search didn't find direct Overlay implementations, and we need to verify cache operations to ensure cleanup is properly handled in all scenarios. Let's check these additional patterns to complete our verification.
Cleanup is properly implemented across all disposal scenarios
The cleanup implementation is correctly handled in all necessary scenarios:
- Frame disposal in LRU cache properly calls cleanup on all overlays
- All LRU caches in the codebase implement proper disposal methods:
frame-reader.ts: Calls cleanup on frame overlaysimavid/store.ts: Calls reset on sample frames and destroy on controllersimavid/ima-vid-frame-samples.ts: Handles sample cleanup and index management
The cleanup contract is consistently implemented across the codebase, with proper resource management in cache eviction, clear, and termination scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other disposal points that might need cleanup
rg -A 5 "dispose|clear|terminate" app/packages/looker/src/
# Check if other overlay consumers properly handle cleanup
ast-grep --pattern 'class $_ implements Overlay {
$$$
cleanup() {
$$$
}
$$$
}'
Length of output: 15069
Script:
# Let's search for overlay implementations and their cleanup methods
ast-grep --pattern 'class $_ {
$$$
cleanup() {
$$$
}
$$$
}'
# Search for cache clear/dispose operations
rg -A 3 "new LRUCache" app/packages/looker/src/
Length of output: 1545
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
app/packages/looker/src/worker/pooled-fetch.ts (1)
13-17: Improve constant documentation and state managementA few suggestions for improvement:
- The comment about MAX_CONCURRENT_REQUESTS being arbitrary should include reasoning or empirical evidence
- Consider using a class to encapsulate the state variables
-// note: arbitrary number that seems to work well -const MAX_CONCURRENT_REQUESTS = 100; - -let activeRequests = 0; -const requestQueue: QueueItem[] = []; +/** + * Maximum number of concurrent requests allowed. + * This limit helps prevent: + * - Browser connection limits + * - Memory overhead from too many simultaneous requests + * - Server overload + */ +const MAX_CONCURRENT_REQUESTS = 100; + +class RequestQueue { + private activeRequests = 0; + private queue: QueueItem[] = []; + + // ... rest of the implementation +}app/packages/looker/src/worker/decorated-fetch.ts (2)
19-23: Add JSDoc documentation for the function parametersWhile the parameter types are clear, adding JSDoc documentation would improve the API's usability by describing the purpose of each parameter and providing usage examples.
+/** + * Fetches a resource with linear backoff retry strategy + * @param url - The URL to fetch + * @param opts - Fetch options (headers, method, etc.) + * @param retry - Retry configuration options + * @returns Promise resolving to the fetch Response or null + */ export const fetchWithLinearBackoff = async ( url: string, opts: RequestInit = {}, retry: RetryOptions = { retries: DEFAULT_MAX_RETRIES, delay: DEFAULT_BASE_DELAY, } )
Line range hint
25-48: Consider adding timeout handling and maximum delay limitThe current implementation could benefit from two improvements:
- Handling network timeouts explicitly
- Adding a maximum delay limit to prevent excessive wait times
Consider applying these enhancements:
export interface RetryOptions { retries: number; delay: number; + timeout?: number; + maxDelay?: number; } export const fetchWithLinearBackoff = async ( url: string, opts: RequestInit = {}, retry: RetryOptions = { retries: DEFAULT_MAX_RETRIES, delay: DEFAULT_BASE_DELAY, + timeout: 30000, + maxDelay: 10000, } ) => { for (let i = 0; i < retry.retries; i++) { try { - const response = await fetch(url, opts); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), retry.timeout); + const response = await fetch(url, { ...opts, signal: controller.signal }); + clearTimeout(timeoutId); if (response.ok) { // ... } } catch (e) { // ... if (i < retry.retries - 1) { + const delay = Math.min(retry.delay * (i + 1), retry.maxDelay ?? Infinity); await new Promise((resolve) => - setTimeout(resolve, retry.delay * (i + 1)) + setTimeout(resolve, delay) ); } } } };app/packages/looker/src/worker/pooled-fetch.test.ts (4)
7-15: Consider improving type safety in the helper function.The non-null assertion operator (!) in the return statement could be replaced with runtime checks for better type safety.
function createDeferredPromise<T>() { let resolve: (value: T | PromiseLike<T>) => void; let reject: (reason?: any) => void; const promise = new Promise<T>((res, rej) => { resolve = res; reject = rej; }); - return { promise, resolve: resolve!, reject: reject! }; + if (!resolve || !reject) { + throw new Error('Promise executor not called synchronously'); + } + return { promise, resolve, reject }; }
34-57: Consider enhancing the order test robustness.While the test verifies basic ordering, it could be more comprehensive by:
- Testing with more than two requests
- Verifying that subsequent requests don't start before previous ones complete
it("should process multiple requests in order", async () => { - const mockResponse1 = new Response("First", { status: 200 }); - const mockResponse2 = new Response("Second", { status: 200 }); + const responses = Array.from({ length: 4 }, (_, i) => + new Response(`Response ${i}`, { status: 200 })); - const deferred1 = createDeferredPromise<Response>(); - const deferred2 = createDeferredPromise<Response>(); + const deferreds = responses.map(() => createDeferredPromise<Response>()); + let fetchCallCount = 0; mockedFetch - .mockImplementationOnce(() => deferred1.promise) - .mockImplementationOnce(() => deferred2.promise); + .mockImplementation(() => { + const current = fetchCallCount++; + return deferreds[current].promise; + }); - const promise1 = enqueueFetch({ url: "https://fiftyone.ai/1" }); - const promise2 = enqueueFetch({ url: "https://fiftyone.ai/2" }); + const promises = responses.map((_, i) => + enqueueFetch({ url: `https://fiftyone.ai/${i}` })); - deferred1.resolve(mockResponse1); + // Verify only first request started + expect(fetchCallCount).toBe(1); - const response1 = await promise1; - expect(response1).toBe(mockResponse1); + // Resolve promises in order + for (let i = 0; i < responses.length; i++) { + deferreds[i].resolve(responses[i]); + const response = await promises[i]; + expect(response).toBe(responses[i]); - deferred2.resolve(mockResponse2); - - const response2 = await promise2; - expect(response2).toBe(mockResponse2); + // Verify next request started + if (i < responses.length - 1) { + expect(fetchCallCount).toBe(i + 2); + } + } });
59-83: LGTM! Comprehensive concurrency test.The test effectively verifies the concurrency limit. Consider adding verification that new requests start immediately after previous ones complete.
// resolve all deferred promises deferredPromises.forEach((deferred, index) => { deferred.resolve(new Response(`Response ${index}`, { status: 200 })); + // Verify next batch of requests started + if (index < numRequests - MAX_CONCURRENT_REQUESTS) { + expect(mockedFetch).toHaveBeenCalledTimes( + Math.min(index + MAX_CONCURRENT_REQUESTS + 1, numRequests) + ); + } });
85-110: Consider enhancing retry test coverage.While the tests verify basic error handling, they could be improved by:
- Verifying increasing delays between retries
- Testing successful retry scenarios
it("should retry on retryable errors up to MAX_RETRIES times", async () => { const MAX_RETRIES = 3; + const BASE_DELAY = 50; + const startTimes: number[] = []; + + vi.useFakeTimers(); mockedFetch.mockRejectedValue(new Error("Network Error")); + mockedFetch.mockImplementation(() => { + startTimes.push(Date.now()); + return Promise.reject(new Error("Network Error")); + }); await expect( enqueueFetch({ url: "https://fiftyone.ai", retryOptions: { retries: MAX_RETRIES, - delay: 50, + delay: BASE_DELAY, }, }) ).rejects.toThrow("Max retries for fetch reached"); expect(mockedFetch).toHaveBeenCalledTimes(MAX_RETRIES); + + // Verify exponential backoff + for (let i = 1; i < startTimes.length; i++) { + const delay = startTimes[i] - startTimes[i - 1]; + expect(delay).toBeGreaterThanOrEqual(BASE_DELAY * Math.pow(2, i - 1)); + } + + vi.useRealTimers(); }); + + it("should succeed after retrying", async () => { + const mockResponse = new Response("OK", { status: 200 }); + mockedFetch + .mockRejectedValueOnce(new Error("Network Error")) + .mockRejectedValueOnce(new Error("Network Error")) + .mockResolvedValueOnce(mockResponse); + + const response = await enqueueFetch({ + url: "https://fiftyone.ai", + retryOptions: { + retries: 3, + delay: 50, + }, + }); + + expect(response).toBe(mockResponse); + expect(mockedFetch).toHaveBeenCalledTimes(3); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/packages/looker/src/worker/decorated-fetch.ts(3 hunks)app/packages/looker/src/worker/pooled-fetch.test.ts(1 hunks)app/packages/looker/src/worker/pooled-fetch.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/looker/src/worker/decorated-fetch.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/pooled-fetch.test.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/pooled-fetch.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (4)
app/packages/looker/src/worker/pooled-fetch.ts (1)
1-11: Verify RetryOptions implementation in decorated-fetch
The interface is well-structured, but let's verify the RetryOptions implementation to ensure proper type safety.
✅ Verification successful
RetryOptions implementation is properly defined and used
The RetryOptions interface in decorated-fetch.ts is well-implemented with:
- Required
retriesanddelaynumber fields - Consistent usage across both files
- Proper type safety in the import chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check RetryOptions implementation and usage
ast-grep --pattern 'type RetryOptions = {
$$$
}'
# Check for any other usage of RetryOptions
rg "RetryOptions" -A 3
Length of output: 1269
app/packages/looker/src/worker/decorated-fetch.ts (2)
6-9: LGTM: Well-structured interface definition
The RetryOptions interface provides a clean and type-safe way to configure retry behavior.
25-27: Verify retry behavior with different content types
Since this fetch utility is used for transferring image data (as mentioned in PR objectives), we should ensure it properly handles various content types and response sizes.
✅ Verification successful
Let me gather more information about the content types and response handling.
Let me try a different approach to check the implementation details.
Retry behavior implementation is properly tested for different response types
The implementation and tests show comprehensive handling of various scenarios:
- Successful responses (200)
- Non-retryable client errors (4xx status codes)
- Server errors (5xx status codes)
- Network errors with linear backoff
- Proper response validation through
response.ok
The utility is well-designed to handle different content types as it:
- Returns the raw Response object, allowing the caller to handle any content type
- Doesn't make assumptions about the response body format
- Properly distinguishes between retryable and non-retryable errors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all fetch calls using this utility to verify content types
rg -A 2 "fetchWithLinearBackoff\(" --type ts
Length of output: 1999
Script:
#!/bin/bash
# Check for content type handling in the fetch utility and its tests
rg -B 2 -A 4 "content-type|Content-Type" --type ts app/packages/looker/src/worker/
# Look for the actual implementation of response handling
ast-grep --pattern 'const response = await fetch($_, $_)
$$$'
Length of output: 150
Script:
#!/bin/bash
# Check the full implementation of decorated-fetch.ts
cat app/packages/looker/src/worker/decorated-fetch.ts
# Check test cases for response handling
cat app/packages/looker/src/worker/decorated-fetch.test.ts
Length of output: 4796
app/packages/looker/src/worker/pooled-fetch.test.ts (1)
26-32: LGTM! Well-structured success test.
The test follows the AAA pattern and effectively verifies the basic functionality.
What changes are proposed in this pull request?
Continues efforts in #5156 to improve FiftyOne's overlays rendering performance.
Instead of transferring raw 32 bit image array buffers from worker to the main thread, we create ImageBitmap in the worker and transfer that instead.
Also adds a pooled fetch executor to make sure worker threads don't inundate the browser with unbounded number of fetch requests.
BEFORE
Worker
Main thread
Overlay constructor
ImageDatato the ephemeral canvasdrawMaskNOW
Worker
Main thread
Overlay constructor
drawMaskImageBitmapfrom worker to main canvasHow is this patch tested? If it is not, please explain why.
Locally. Todo: more rigorous performance testing
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
fetchWithLinearBackofffunction to ensure consistent usage of options.Refactor
processLabelsandprocessSamplefunctions to accommodate new processing logic.disposefunction for better cleanup of overlays.