Docker: Respect SE_SCREEN_{WIDTH,HEIGHT} variables in ffmpeg recording

[StenAL]

User description

Description

Currently, these variables are always overridden in the call the wait_for_display(). Before the upgrade to ffmpeg-7.0.2 (PR #2374), the SE_SCREEN_WIDTH and SE_SCREEN_HEIGHT variables were respected in the case where no video uploading was done because wait_for_display() was never called. After the upgrade to 7.0.2, the function is always called and overrides the VIDEO_SIZE variable.

Now, if both screen width and height are specified, VIDEO_SIZE is not overridden and users can control the recording size again.

Motivation and Context

In versions earlier than selenium/video:ffmpeg-7.0.2, the dimensions for recordings are SE_SCREEN_WIDTH x SE_SCREEN_HEIGHT of the selenium/video container. In later versions, the dimensions are SE_SCREEN_WIDTH x SE_SCREEN_HEIGHT of the Selenium node container. This breaks some workflows because of the unexpected recording size.

To reproduce the issue, this docker-compose.yml file can be started and then stopped:

services:
    browser:
        image: selenium/standalone-chrome:132.0
        environment:
            - SE_SCREEN_WIDTH=1920
            - SE_SCREEN_HEIGHT=150

    browser_recording:
        image: selenium/video:ffmpeg-7.0.2-20240829
        user: root
        volumes:
            - .:/videos
        environment:
            - SE_SCREEN_WIDTH=306
            - SE_SCREEN_HEIGHT=46
            - DISPLAY_CONTAINER_NAME=browser

When using selenium/video:ffmpeg-7.0.2-20240829 or newer images, the recording size is 1920x150 px but with selenium/video:ffmpeg-7.0.1-20240820, the recording is 306x46 px. With these changes the recording is once again 306x46 px.

If either of SE_SCREEN_WIDTH or SE_SCREEN_HEIGHT is missing then the behavior falls back to using the node's resolution.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Bug fix

---

Description

• Ensure SE_SCREEN_WIDTH and SE_SCREEN_HEIGHT variables control recording size.

• Prevent overriding VIDEO_SIZE when screen dimensions are specified.

• Remove default screen width and height from Dockerfile environment variables.

---

Changes walkthrough 📝

	Relevant files
Bug fix	video.sh Conditional handling of `VIDEO_SIZE` in `video.sh`              Video/video.sh Added conditional check for SE_SCREEN_WIDTH and SE_SCREEN_HEIGHT. Prevented overriding VIDEO_SIZE if screen dimensions are set. +3/-1
Configuration changes	Dockerfile Removed default screen dimensions from Dockerfile                Video/Dockerfile Removed default values for SE_SCREEN_WIDTH and SE_SCREEN_HEIGHT. Simplified environment variable setup for screen dimensions. +0/-2

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[CLAassistant]

All committers have signed the CLA.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 Security concerns Command Injection: The script uses unquoted variables in shell commands and awk expressions which could potentially allow command injection if the environment variables contain malicious values. The DISPLAY_CONTAINER_NAME and other variables should be properly quoted when used in commands.

⚡ Recommended focus areas for review Error Handling The script should validate that SE_SCREEN_WIDTH and SE_SCREEN_HEIGHT contain valid positive integer values when they are set, to avoid potential ffmpeg recording issues with invalid dimensions if [ -z "$SE_SCREEN_WIDTH" -o -z "$SE_SCREEN_HEIGHT" ]; then VIDEO_SIZE=$(xdpyinfo | grep 'dimensions:' | awk '{print $2}') fi

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Ensure variable is always defined Set VIDEO_SIZE explicitly when using SE_SCREEN dimensions to ensure the variable is always defined Video/video.sh [74-76] if [ -z "$SE_SCREEN_WIDTH" -o -z "$SE_SCREEN_HEIGHT" ]; then VIDEO_SIZE=$(xdpyinfo | grep 'dimensions:' | awk '{print $2}') +else + VIDEO_SIZE="${SE_SCREEN_WIDTH}x${SE_SCREEN_HEIGHT}" fi Apply this suggestion Suggestion importance[1-10]: 9 Why: This suggestion fixes a critical issue where VIDEO_SIZE might be undefined when SE_SCREEN dimensions are provided, which could cause the script to fail or behave unexpectedly when displaying dimensions.	9
	Validate screen dimension inputs Add validation for SE_SCREEN_WIDTH and SE_SCREEN_HEIGHT values to ensure they are positive integers when provided Video/video.sh [74-76] if [ -z "$SE_SCREEN_WIDTH" -o -z "$SE_SCREEN_HEIGHT" ]; then VIDEO_SIZE=$(xdpyinfo | grep 'dimensions:' | awk '{print $2}') +else + if [[ "$SE_SCREEN_WIDTH" =~ ^[0-9]+$ ]] && [[ "$SE_SCREEN_HEIGHT" =~ ^[0-9]+$ ]] && [ "$SE_SCREEN_WIDTH" -gt 0 ] && [ "$SE_SCREEN_HEIGHT" -gt 0 ]; then + VIDEO_SIZE="${SE_SCREEN_WIDTH}x${SE_SCREEN_HEIGHT}" + else + echo "Error: SE_SCREEN_WIDTH and SE_SCREEN_HEIGHT must be positive integers" + exit 1 + fi fi Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion adds crucial input validation for screen dimensions, preventing potential errors from invalid values and making the code more robust by ensuring only positive integers are accepted.	8

[VietND96]

Thank you, @StenAL!