get dynamic groups target frame rate from app config instead of harcoded value #5368
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant UI as User Interface
participant State as Recoil State
participant Looker as ImaVidLooker
participant Controller as ImaVidFramesController
UI->>State: Set dynamicGroupsTargetFrameRate
State->>Looker: Provide dynamicGroupsTargetFrameRate
Looker->>Controller: Initialize with targetFrameRate
Controller-->>Looker: Configure rendering parameters
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
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
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/packages/looker/src/lookers/imavid/controller.ts (1)
186-187: Consider documenting the frame rate limit change.The maximum frame rate has been increased from 24 to 60 FPS, which allows for smoother playback on capable displays. Consider adding a code comment explaining this change and its benefits.
Also applies to: 194-194
fiftyone/server/query.py (1)
214-214: Add docstring documentation for the new field.The new field
dynamic_groups_target_frame_ratelacks docstring documentation. Consider adding a description to maintain consistency with other fields in the class.+ """The target frame rate when rendering ordered dynamic groups of images as videos.""" dynamic_groups_target_frame_rate: int = 30
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
app/packages/app/src/pages/datasets/__generated__/DatasetPageQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/fragments/__generated__/datasetAppConfigFragment.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/queries/__generated__/datasetQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**
📒 Files selected for processing (12)
app/packages/core/src/components/Modal/ImaVidLooker.tsx(2 hunks)app/packages/looker/src/elements/imavid/index.ts(4 hunks)app/packages/looker/src/lookers/imavid/constants.ts(0 hunks)app/packages/looker/src/lookers/imavid/controller.ts(6 hunks)app/packages/looker/src/util.ts(1 hunks)app/packages/relay/src/fragments/datasetAppConfigFragment.ts(1 hunks)app/packages/state/src/hooks/useCreateLooker.ts(4 hunks)app/packages/state/src/recoil/selectors.ts(1 hunks)app/packages/state/src/recoil/types.ts(1 hunks)app/schema.graphql(1 hunks)fiftyone/core/odm/dataset.py(2 hunks)fiftyone/server/query.py(1 hunks)
💤 Files with no reviewable changes (1)
- app/packages/looker/src/lookers/imavid/constants.ts
🧰 Additional context used
📓 Path-based instructions (8)
app/packages/state/src/recoil/types.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/relay/src/fragments/datasetAppConfigFragment.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/state/src/recoil/selectors.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/core/src/components/Modal/ImaVidLooker.tsx (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/util.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/state/src/hooks/useCreateLooker.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/lookers/imavid/controller.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/elements/imavid/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 (14)
app/packages/relay/src/fragments/datasetAppConfigFragment.ts (1)
9-9: LGTM!The new field follows the existing naming convention and is correctly positioned in the fragment.
app/packages/state/src/recoil/types.ts (1)
122-122: LGTM!The new property is well-typed and maintains consistency with the GraphQL schema.
app/packages/looker/src/lookers/imavid/controller.ts (1)
17-17: LGTM!The refactoring from
frameRatetotargetFrameRateis consistent and well-implemented.Also applies to: 35-35, 44-44
app/packages/state/src/hooks/useCreateLooker.ts (1)
66-68: LGTM!The integration of the dynamic frame rate configuration is well-implemented, following React and Recoil best practices.
Also applies to: 234-234, 244-244
app/packages/core/src/components/Modal/ImaVidLooker.tsx (3)
40-42: LGTM! Clean introduction of the frame rate configuration.The new Recoil state variable is well-integrated and follows React hooks best practices.
226-227: LGTM! Proper integration of dynamic frame rate in timeline config.The frame rate is correctly incorporated into the timeline configuration object, with appropriate dependency tracking in the useMemo hook.
Line range hint
451-458: LGTM! Proper handling of frame rate and timeout delay.The frame rate initialization and timeout delay calculation are implemented correctly, with appropriate conditional checks to prevent unnecessary recalculations.
app/packages/looker/src/util.ts (1)
256-261: LGTM! Clean function signature update.The function has been properly modified to accept a dynamic frame rate parameter while maintaining the correct calculation logic.
app/packages/state/src/recoil/selectors.ts (1)
160-165: LGTM! Well-structured Recoil selector.The selector is properly implemented with:
- Correct TypeScript typing
- Appropriate default value handling
- Consistent naming convention
app/packages/looker/src/elements/imavid/index.ts (3)
83-84: LGTM! Well-defined class properties.The new private properties are properly typed and follow TypeScript best practices.
351-353: LGTM! Proper adaptation of look-ahead calculation.The offset calculation has been correctly updated to use the dynamic frame rate while maintaining the original logic structure.
453-458: LGTM! Proper timeout delay calculation update.The timeout delay calculation has been correctly updated with:
- Appropriate conditional check for recalculation
- Correct parameter passing to getMillisecondsFromPlaybackRate
fiftyone/core/odm/dataset.py (1)
530-531: LGTM!The new field is well-documented and properly implemented with appropriate type and default value.
Also applies to: 550-550
app/schema.graphql (1)
251-251: LGTM!The GraphQL schema change is correctly implemented with appropriate type constraints and naming convention.
| @@ -181,15 +183,15 @@ export class ImaVidFramesController { | |||
| } | |||
|
|
|||
| public setFrameRate(newFrameRate: number) { | |||
| if (newFrameRate > 24) { | |||
| throw new Error("max frame rate is 24"); | |||
| if (newFrameRate > 60) { | |||
There was a problem hiding this comment.
just curious, is there a technical reason to cap at 60?
There was a problem hiding this comment.
no... it's arbitrary based on local testing. but i'm hoping to set the right expectation by enforcing this cap. in ImaVid, we're on-the-fly fetching and streaming images and achieving higher fps than even 30 is kind of hard
What changes are proposed in this pull request?
Right now target frame rate for imavid is set to 30. This PR exposes a dataset app config property called
dynamicGroupsTargetFrameRatewhich allows customers to declare it per dataset. The default is still 30.How is this patch tested? If it is not, please explain why.
In the app, select "Render frames as video" and observe that frame rate is ~10. In a shell or using an operator from the UI, change the target frame rate again, refresh the app, and see that it's different. Verify it both in the grid and in the modal.
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
New Features
Improvements
Technical Updates