Skip to content

Improve gif player performance with vsync support and frame rate control#42

Open
Copilot wants to merge 3 commits intodevfrom
copilot/fix-36
Open

Improve gif player performance with vsync support and frame rate control#42
Copilot wants to merge 3 commits intodevfrom
copilot/fix-36

Conversation

Copy link
Copy Markdown

Copilot AI commented Jun 4, 2025

This PR significantly improves the gif player implementation by adding VEX render pipeline integration, configurable frame rate control, and performance optimizations.

Key Improvements

🔄 VEX Render Pipeline Integration

  • Added proper Brain.Screen.render(bSyncWait, true) calls with configurable vsync
  • Integrates with VEX's double buffering system for smoother rendering
  • Uses ConfigManager.getVsyncGif() setting by default with runtime override support

⚡ Performance Optimizations

  • 30 FPS frame cap by default for optimal VEX brain performance
  • Eliminated memcpy overhead in hot rendering paths with direct color assignment
  • Added buffer validation to prevent rendering with invalid data
  • Memory bounds checking and better error handling
  • Buffer clearing to prevent visual artifacts

🎛️ Enhanced Configuration

// Uses config settings by default
vex::Gif gif("assets/auto.gif", 0, 0);

// Or explicit control
vex::Gif gif("assets/driver.gif", 0, 0, true, 30); // vsync on, 30 FPS cap

// Runtime adjustments
gif.setVsync(false);
gif.setMaxFps(60);

🛡️ Better Error Handling

  • Dimension validation to prevent excessive memory allocation (max 4096x4096)
  • Improved memory allocation error handling with proper cleanup
  • Enhanced file reading safety checks

API Changes

New Constructor:

Gif(const char *fname, int sx, int sy, bool enable_vsync, int max_fps = 0);

New Methods:

  • setVsync(bool enable) - Toggle vsync at runtime
  • setMaxFps(int fps) - Adjust frame rate limit (0 = no limit)

Performance Impact

The improvements deliver:

  • Smoother frame playback through proper vsync integration
  • Reduced CPU overhead with optimized rendering loops
  • Configurable performance vs quality trade-offs
  • Better memory efficiency and safety

Before:

// Old implementation ignored vsync parameter
void gifplayer(bool enableVsync) {
    vex::Gif gif("assets/auto.gif", 0, 0); // vsync ignored
}

After:

// New implementation properly uses configuration
void gifplayer(bool enableVsync) {
    bool useVsync = enableVsync || ConfigManager.getVsyncGif();
    vex::Gif gif("assets/auto.gif", 0, 0, useVsync, 30); // integrated
}

Fixes #36.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • http://168.63.129.16:80/machine/
    • Triggering command: /usr/bin/python3 -u bin/WALinuxAgent-2.13.1.1-py3.9.egg -collect-logs (http block)

If you need me to access, download, or install something from one of these locations, you can either:


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits June 4, 2025 13:12
…for gif player

Co-authored-by: Voidless7125 <167136334+Voidless7125@users.noreply.github.com>
…f player

Co-authored-by: Voidless7125 <167136334+Voidless7125@users.noreply.github.com>
Copilot AI changed the title [WIP] Improve gif player code Improve gif player performance with vsync support and frame rate control Jun 4, 2025
Copilot AI requested a review from Voidless7125 June 4, 2025 13:15
@Voidless7125 Voidless7125 marked this pull request as ready for review June 17, 2025 19:25
Copilot AI review requested due to automatic review settings June 17, 2025 19:25
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the GIF player by integrating VEX vsync support, adding configurable frame‐rate control, and optimizing rendering performance.

  • Introduces enable_vsync and max_fps parameters with defaults from ConfigManager
  • Replaces memcpy in hot paths with direct color assignment and adds buffer/dimension checks
  • Adds frame‐rate limiting logic and proper use of Brain.Screen.render() for smoother playback

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/functions.cpp Integrate vsync/config and apply a 30 FPS default cap
src/display/gifplayer/gifplayer.cpp Add emu_read checks, dimension validation, optimized rendering and frame timing logic
include/display/gifdec.h Expose new constructors and setters with Doxygen stubs
Comments suppressed due to low confidence (2)

include/display/gifdec.h:112

  • The setVsync and setMaxFps methods should have Doxygen comments explaining their parameters and behavior to match the style of the constructors.
        void setVsync(bool enable) { _enable_vsync = enable; }

src/functions.cpp:206

  • The standalone if breaks the original else if chain, causing later branches to always be evaluated after the autonomous loop. Change it back to else if to preserve mutual exclusivity.
    if (Competition.isAutonomous())

Comment on lines +49 to 52
if (!emu_memory || !buffer || len == 0)
{
return -1;
}
Copy link

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Returning -1 for len == 0 may be misleading since reading zero bytes usually returns 0. Consider returning 0 to represent EOF or no bytes read.

Suggested change
if (!emu_memory || !buffer || len == 0)
{
return -1;
}
if (!emu_memory || !buffer)
{
return -1;
}
if (len == 0)
{
return 0;
}

Copilot uses AI. Check for mistakes.
height = read_num(fd);

// Validate dimensions to prevent excessive memory allocation
if (width == 0 || height == 0 || width > 4096 || height > 4096)
Copy link

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The literal 4096 is a magic number for max dimensions. Extract it into a named constant (e.g., kMaxGifDimension) to improve readability and ease future maintenance.

Suggested change
if (width == 0 || height == 0 || width > 4096 || height > 4096)
if (width == 0 || height == 0 || width > kMaxGifDimension || height > kMaxGifDimension)

Copilot uses AI. Check for mistakes.
gif->width, gif->height);

// Use VEX render API with vsync support
if (instance->_enable_vsync)
Copy link

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessing _enable_vsync and _max_fps from both the main thread and the render thread can cause data races. Use std::atomic or a mutex to synchronize these shared fields.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve gif player code

3 participants