Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update: Rollback FFmpeg v6.1.1 #2247

Merged
merged 1 commit into from
May 4, 2024
Merged

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented May 4, 2024

User description

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

update: Rollback FFmpeg v6.1.1

Motivation and Context

  • With the video docker image built on top of FFmpeg 7.0, there was a report that the video file integrity failed. The video has file size but could not be opened and viewed.
  • From image tag based 4.20.0 onwards, the video Docker image is based on the FFmpeg Ubuntu image provided by
    linuxserver/docker-ffmpeg project since the image is available for multi-platform. Thank you for simplifying our project and helping us move forward with multiple architecture support.

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.

Type

Bug fix, Enhancement


Description

  • Rolled back FFmpeg version to 6.1.1 across various configurations and Docker Compose files due to issues with video file integrity.
  • Updated video testing in Selenium tests to use Flowplayer and enhanced video play status checks.
  • Updated documentation and Helm chart values to reflect the changes in FFmpeg version.
  • Added acknowledgment for the linuxserver/docker-ffmpeg project in the README.

Changes walkthrough

Relevant files
Enhancement
__init__.py
Update video testing to use Flowplayer and enhance checks

tests/SeleniumTests/init.py

  • Updated the video test to use a new video player (Flowplayer) and
    added checks for video play status.
  • +8/-4     
    Bug fix
    Makefile
    Rollback FFmpeg version in Makefile and adjust Docker commands

    Makefile

  • Changed FFmpeg version back to 6.1.1 and updated the Docker run
    command to include an entrypoint adjustment.
  • +6/-6     
    Configuration changes
    config.toml
    Update Docker image tag for video recording in config       

    NodeDocker/config.toml

  • Updated the Docker image tag for video recording to use FFmpeg version
    6.1.1.
  • +1/-1     
    values.yaml
    Update Helm chart values for FFmpeg version                           

    charts/selenium-grid/values.yaml

    • Updated the video image tag in Helm chart values to FFmpeg 6.1.1.
    +2/-2     
    docker-compose-v3-video-upload.yml
    Update Docker Compose for video upload with FFmpeg version

    docker-compose-v3-video-upload.yml

    • Updated the Docker image for video services to FFmpeg 6.1.1.
    +3/-3     
    docker-compose-v3-video.yml
    Update Docker Compose for video services with FFmpeg version

    docker-compose-v3-video.yml

    • Updated the Docker image for video services to FFmpeg 6.1.1.
    +3/-3     
    Documentation
    README.md
    Documentation update for FFmpeg version rollback                 

    README.md

  • Updated documentation to reflect the rollback to FFmpeg version 6.1.1.
  • Added acknowledgment for the linuxserver/docker-ffmpeg project.
  • +9/-5     
    README.md
    Update Helm chart README with new FFmpeg version                 

    charts/selenium-grid/README.md

  • Updated the global configuration and video recorder image tags to
    FFmpeg 6.1.1.
  • +15/-15 

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    Copy link

    PR Description updated to latest commit (3561658)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are mostly version rollbacks and updates to URLs and image tags, which are straightforward to verify. However, the changes in the test scripts and Makefile commands require careful attention to ensure they align with the new versions and functionality.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Possible Bug: The changes in the test script for playing videos might not correctly handle the new video player's behavior. It's important to ensure that the new test logic accurately reflects the player's operations and that all scenarios are covered.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Maintainability
    Replace hardcoded strings with variables for easier maintenance.

    Replace the hardcoded tag name 'video' with a variable to avoid repetition and facilitate
    future changes.

    tests/SeleniumTests/init.py [77]

    -video = driver.find_element(By.TAG_NAME, 'video')
    +video_tag = 'video'
    +video = driver.find_element(By.TAG_NAME, video_tag)
     
    Use a variable for repeated external tool references to simplify updates and reduce errors.

    Consolidate the repeated use of $(FFMPEG_BASED_NAME)/ffmpeg:$(FFMPEG_BASED_TAG) into a
    single variable to reduce redundancy and potential errors in future modifications.

    Makefile [479]

    -docker run -u $$(id -u) -v $$(pwd):$$(pwd) -w $$(pwd) --entrypoint="" $(FFMPEG_BASED_NAME)/ffmpeg:$(FFMPEG_BASED_TAG) ffmpeg -v error -i ./tests/videos/chrome_video.mp4 -f null - 2>error.log
    +FFMPEG_DOCKER_IMAGE := $(FFMPEG_BASED_NAME)/ffmpeg:$(FFMPEG_BASED_TAG)
    +docker run -u $$(id -u) -v $$(pwd):$$(pwd) -w $$(pwd) --entrypoint="" $(FFMPEG_DOCKER_IMAGE) ffmpeg -v error -i ./tests/videos/chrome_video.mp4 -f null - 2>error.log
     
    Best practice
    Improve element selection specificity in Selenium tests for reliability.

    Use a more specific selector than By.TAG_NAME for finding the play button to improve test
    reliability and avoid potential issues with multiple elements matching the same tag.

    Makefile [73-75]

     play_button = wait.until(
    -    EC.element_to_be_clickable((By.TAG_NAME, 'flowplayer-play-icon'))
    +    EC.element_to_be_clickable((By.CSS_SELECTOR, '.flowplayer .play-button'))
     )
     
    Remove unnecessary Docker command options to prevent unexpected behavior.

    Avoid using the --entrypoint="" option if not necessary, as it can lead to unexpected
    behavior if the Docker image is designed to run with a specific entrypoint.

    Makefile [479]

    -docker run -u $$(id -u) -v $$(pwd):$$(pwd) -w $$(pwd) --entrypoint="" $(FFMPEG_BASED_NAME)/ffmpeg:$(FFMPEG_BASED_TAG) ffmpeg -v error -i ./tests/videos/chrome_video.mp4 -f null - 2>error.log
    +docker run -u $$(id -u) -v $$(pwd):$$(pwd) -w $$(pwd) $(FFMPEG_BASED_NAME)/ffmpeg:$(FFMPEG_BASED_TAG) ffmpeg -v error -i ./tests/videos/chrome_video.mp4 -f null - 2>error.log
     
    Enhancement
    Simplify variable assignments in Makefile for clarity.

    Replace the use of $(or ...) with direct variable assignments for clarity and to avoid
    redundancy, since $(or ...) is not needed when the variable is being defined for the first
    time or reassigned.

    Makefile [20]

    -FFMPEG_TAG_VERSION := $(or $(FFMPEG_TAG_VERSION),$(FFMPEG_TAG_VERSION),ffmpeg-6.1.1)
    +FFMPEG_TAG_VERSION := ffmpeg-6.1.1
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @VietND96 VietND96 merged commit b079935 into SeleniumHQ:trunk May 4, 2024
    11 checks passed
    @VietND96 VietND96 deleted the rollback-ffmpeg branch May 4, 2024 14:43
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    1 participant