renderdiff: enable update goldens on commit merge (#8771)

poweifeng · web-flow · commit 3da7dabb2ac4 · 2025-06-02T14:12:26.000-07:00
diff --git a/.github/actions/get-commit-msg/action.yml b/.github/actions/get-commit-msg/action.yml
@@ -0,0 +1,33 @@
+name: 'Get commit message'
+outputs:
+  msg:
+    value: ${{ steps.action_output.outputs.msg }}
+runs:
+  using: "composite"
+  steps:
+    - name: Find commit message (on push)
+      if: github.event_name == 'push'
+      shell: bash
+      run: |
+        echo "${{ github.event.head_commit.message }}" >> /tmp/commit_msg.txt
+    - name: Checkout code
+      shell: bash
+      id: checkout_code
+      if: github.event_name == 'pull_request'
+      run: |
+        echo "hash=$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT"
+        git checkout ${{ github.event.pull_request.head.ref }}
+        COMMIT_MESSAGE=$(git log -1)
+        echo "${COMMIT_MESSAGE}" >> /tmp/commit_msg.txt
+    - shell: bash
+      id: action_output
+      run: |
+        DELIMITER="EOF_FILE_CONTENT_$(date +%s)" # Using timestamp to make it more unique
+        echo "msg<<$DELIMITER" >> "$GITHUB_OUTPUT"
+        cat /tmp/commit_msg.txt >> "$GITHUB_OUTPUT"
+        echo "$DELIMITER" >> "$GITHUB_OUTPUT"
+    - name: Uncheckout code
+      shell: bash
+      if: github.event_name == 'pull_request'
+      run: |
+        git checkout ${{ steps.checkout.outputs.hash }}
diff --git a/.github/workflows/postsubmit.yml b/.github/workflows/postsubmit.yml
@@ -0,0 +1,38 @@
+name: 'Post-submit tasks'
+
+on:
+  pull_request:
+    branches:
+      - main
+  push:
+    branches:
+      - main
+
+jobs:
+  update-renderdiff-goldens:
+    name: update-renderdiff-goldens
+    runs-on: 'ubuntu-24.04-4core'
+    steps:
+      - uses: actions/checkout@v4.1.6
+        with:
+          fetch-depth: 0
+      - uses: ./.github/actions/linux-prereq
+      - id: get_commit_msg
+        uses: ./.github/actions/get-commit-msg
+      - name: Prerequisites
+        run: pip install tifffile numpy
+      - name: Run update script
+        env:
+          GH_TOKEN: ${{ secrets.FILAMENTBOT_TOKEN }}
+        run: |
+          GOLDEN_BRANCH=$(echo "${{ steps.get_commit_msg.outputs.msg }}" | python3 test/renderdiff/src/commit_msg.py)
+          COMMIT_HASH=$(echo "${{ steps.get_commit_msg.outputs.msg }}" | head -n 1 | tr -d 'commit ')
+          if [[ "${GOLDEN_BRANCH}" != "main" ]]; then
+              git config --global user.email "filament.bot@gmail.com"
+              git config --global user.name "Filament Bot"
+              git config --global credential.helper cache
+              echo "branch==${GOLDEN_BRANCH}"
+              echo "hash==${COMMIT_HASH}"
+              python3 test/renderdiff/src/update_golden.py --branch=${GOLDEN_BRANCH} \
+                  --merge-to-main --filament-tag=${COMMIT_HASH} --golden-repo-token=${GH_TOKEN}
+          fi
diff --git a/.github/workflows/presubmit.yml b/.github/workflows/presubmit.yml
@@ -3,10 +3,10 @@ name: Presubmit
 on:
   push:
     branches:
-    - main
+      - main
   pull_request:
     branches:
-    - main
+      - main
 
 jobs:
   build-desktop-mac:
@@ -110,31 +110,35 @@ jobs:
           fetch-depth: 0
       - name: Check for manual edits to /docs
         run: |
-           echo "${{ github.event.pull_request.head.sha }} -- ${{ github.event.pull_request.head.sha }}"
-           # disable for now
-           # bash docs_src/build/presubmit_check.sh ${{ github.event.pull_request.head.sha }}
+          echo "${{ github.event.pull_request.head.sha }} -- ${{ github.event.pull_request.head.sha }}"
+          # disable for now
+          # bash docs_src/build/presubmit_check.sh ${{ github.event.pull_request.head.sha }}
 
   test-renderdiff:
     name: test-renderdiff
     runs-on: macos-14-xlarge
     steps:
+      - uses: actions/checkout@v4.1.6
+        with:
+          fetch-depth: 0
+      - id: get_commit_msg
+        uses: ./.github/actions/get-commit-msg
       - uses: actions/checkout@v4.1.6
         with:
           fetch-depth: 0
       - uses: ./.github/actions/mac-prereq
       - name: Cache Mesa and deps
-        id: mesa-cache
         uses: actions/cache@v4
         with:
           path: mesa
           key: ${{ runner.os }}-mesa-deps-2-${{ vars.MESA_VERSION }}
       - name: Prerequisites
-        id: prereqs
         run: |
           bash build/common/get-mesa.sh
           pip install tifffile numpy
       - name: Run Test
-        run: bash test/renderdiff/test.sh
+        run: |
+          echo "${{ steps.get_commit_msg.outputs.msg }}" | bash test/renderdiff/test.sh
       - uses: actions/upload-artifact@v4
         with:
           name: presubmit-renderdiff-result
diff --git a/build/common/get-mesa.sh b/build/common/get-mesa.sh
@@ -41,6 +41,8 @@ for cmd in "${NEEDED_PYTHON_DEPS[@]}"; do
 done
 deactivate
 
+LOCAL_PKG_CONFIG_PATH=
+
 # Install system deps
 if [[ "$OS_NAME" == "Linux" ]]; then
     if [[ "$GITHUB_WORKFLOW" ]]; then
@@ -82,6 +84,9 @@ elif [[ "$OS_NAME" == "Darwin" ]]; then
         fi
     fi
     HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK=true brew install autoconf automake libx11 libxext libxrandr llvm@${LLVM_VERSION} ninja meson pkg-config libxshmfence
+
+    # For reasons unknown, this is necessary for pkg-config to find homebrew's packages
+    LOCAL_PKG_CONFIG_PATH="/opt/homebrew/lib/pkgconfig:$PKG_CONFIG_PATH"
 fi # [[ "$OS_NAME" == x ]]
 
 LOCAL_LDFLAGS=${LDFLAGS}
@@ -134,9 +139,11 @@ fi
 # -Dgallium-drivers=swrast  => builds GL software rasterizer
 # -Dvulkan-drivers=swrast   => builds VK software rasterizer
 # -Dgallium-drivers=llvmpipe is needed for GL >= 4.1 pipe-screen (see src/gallium/auxiliary/target-helpers/inline_sw_helper.h)
-CXX=${LOCAL_CXX} CC=${LOCAL_CC} PATH=${LOCAL_PATH} LDFLAGS=${LOCAL_LDFLAGS} CPPFLAGS=${LOCAL_CPPFLAGS} \
+PKG_CONFIG_PATH=${LOCAL_PKG_CONFIG_PATH} PATH=${LOCAL_PATH} \
+CXX=${LOCAL_CXX} CC=${LOCAL_CC} LDFLAGS=${LOCAL_LDFLAGS} CPPFLAGS=${LOCAL_CPPFLAGS} \
    meson setup --wipe builddir/ -Dprefix="${MESA_DIR}/out" -Dglx=xlib -Dosmesa=true -Dgallium-drivers=llvmpipe,swrast -Dvulkan-drivers=swrast
-CXX=${LOCAL_CXX} CC=${LOCAL_CC} PATH=${LOCAL_PATH} LDFLAGS=${LOCAL_LDFLAGS} CPPFLAGS=${LOCAL_CPPFLAGS} \
+PKG_CONFIG_PATH=${LOCAL_PKG_CONFIG_PATH} PATH=${LOCAL_PATH} \
+CXX=${LOCAL_CXX} CC=${LOCAL_CC} LDFLAGS=${LOCAL_LDFLAGS} CPPFLAGS=${LOCAL_CPPFLAGS} \
    meson install -C builddir/
 
 # Disable python venv
diff --git a/test/renderdiff/README.md b/test/renderdiff/README.md
@@ -7,11 +7,72 @@ machines. To perform software rasterization, these scripts are centered around [
 rasterizers, but nothing bars us from using another rasterizer like [SwiftShader]. Additionally,
 we should be able to use GPUs where available (though this is more of a future work).
 
-The script `run.py` contains the core logic for taking input parameters (such as the test
-description file) and then running gltf_viewer to produce the results.
+The script `render.py` contains the core logic for taking input parameters (such as the test
+description file) and then running gltf_viewer to produce the renderings.
 
 In the `test` directory is a list of test descriptions that are specified in json.  Please see
 `sample.json` to parse the structure.
 
+## Running the test locally
+ - To run the same presbumit as [`test-renderdiff`][presubmit-renderdiff], you can do
+   ```
+bash test/renderdiff/test.sh
+   ```
+ - This script will generate the renderings based on the current state of your repo.
+   Additionally, it will also compare the generated images with corresponding images in the
+   golden repo.
+ - To just render without running the test, you could use the following script
+   ```
+bash test/renderdiff/generate.sh
+   ```
+
+## Update the golden images
+The golden images are stored in a github repository: https://github.com/google/filament-assets.
+Filament team members should have access to write to the repository. A typical flow for updating
+the goldens is to upload your changed images into **branch** of `filament-assets`. This branch is
+paired with a PR or commit on the `filament` repo.
+
+As an example, imagine I am working on a PR, and I've uploaded my change, which is in a branch
+called `my-pr-branch`, to `filament`. This PR requires updating the golden. We would do it
+in the following fashion
+
+### Using a script to update the golden repo
+
+ - Run interactive mode in the `update_golden.py` script.
+   ```
+python3 test/renderdiff/src/update_golden.py
+   ```
+ - This will guide you through a series of steps to push the changes to a remote branch
+   on `filament-assets`.
+
+### Manually updating the golden repo
+
+ - Check out the golden repo
+   ```
+git clone git@github.com:google/filament-assets.git
+   ```
+ - Create a branch on the golden repo
+   ```
+cd filament-assets
+git switch -c my-pr-branch-golden
+   ```
+ - Copy the new images to their appropriate place in `filament-assets`
+ - Push the `filament-assets` working branch to remote
+   ```
+git push origin my-pr-branch-golden
+   ```
+ - In the commit message of your working branch on `filament`, add the following line
+   ```
+RDIFF_BBRANCH=my-pr-branch-golden
+   ```
+### Manually updating the golden repo
+
+Doing the above has multiple effects:
+ - The presubmit test [`test-renderdiff`][presubmit-renderdiff] will test against the provided
+   branch of the golden repo (i.e. `my-pr-branch-golden`).
+ - If the PR is merged, then there is another workflow that will merge `my-pr-branch-golden` to
+   the `main` branch of the golden repo.
+
 [Mesa]: https://docs.mesa3d.org
-[SwiftShader]: https://github.com/google/swiftshader
+[SwiftShader]: https://github.com/google/swiftshader
+[presubmit-renderdiff]: https://github.com/google/filament/blob/e85dfe75c86106a05019e13ccdbef67e030af675/.github/workflows/presubmit.yml#L118
diff --git a/test/renderdiff/src/commit_msg.py b/test/renderdiff/src/commit_msg.py
@@ -12,11 +12,15 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from utils import execute
+import sys
+import re
 
-def get_last_commit():
-  res, o = execute('git log -1')
-  commit, author, date, _, title, *desc = o.split('\n')
+from utils import execute, ArgParseImpl
+
+RDIFF_UPDATE_GOLDEN_STR = 'RDIFF_BRANCH'
+
+def _parse_commit(commit_str):
+  commit, author, date, _, title, *desc = commit_str.split('\n')
   commit = commit.split(' ')[1]
   title = title.strip()
 
@@ -30,28 +34,27 @@ def get_last_commit():
     desc
   )
 
-def sanitized_split(line, split_atom='\n'):
-  return list(
-    filter(
-      lambda x: len(x) > 0,
-      map(
-        lambda x: x.strip(),
-        line.split(split_atom)
-      )
-    )
-  )
+if __name__ == "__main__":
+  RE_STR = rf"{RDIFF_UPDATE_GOLDEN_STR}(?:S)?=[\[]?([a-zA-Z0-9,\s\-\/]+)[\]]?"
 
-RDIFF_UPDATE_GOLDEN_STR = 'RDIFF_UPDATE_GOLDEN'
+  parser = ArgParseImpl()
+  parser.add_argument('--file', help='A file containing the commit message')
+  args, _ = parser.parse_known_args(sys.argv[1:])
 
-if __name__ == "__main__":
-  RE_STR = f'{RDIFF_UPDATE_GOLDEN_STR}(?:S)?=[\[]?([a-zA-Z0-9,\s]+)[\]]?'
+  if not args.file:
+    msg = sys.stdin.read()
+  else:
+    with open(args.file, 'r') as f:
+      msg = f.read()
 
   to_update = []
-  commit, title, description = get_last_commit()
+  commit, title, description = _parse_commit(msg)
   for line in description:
     m = re.match(RE_STR, line)
     if not m:
       continue
+    print(m.group(1))
+    exit(0)
 
-    to_update += sanitize_split(m.group(1).replace(',', ' '), ' ')
-  print(','.join(to_update))
+  # Always default to the main branch
+  print('main')
diff --git a/test/renderdiff/src/golden_manager.py b/test/renderdiff/src/golden_manager.py
@@ -65,7 +65,7 @@ def _prepare(self):
     assets_dir = self._assets_dir()
     if not os.path.exists(assets_dir):
       execute(
-          f'git clone --depth=1 {self._get_repo_url()}',
+          f'git clone {self._get_repo_url()}',
           cwd=self.working_dir_,
           capture_output=False
       )
@@ -83,13 +83,26 @@ def update(self):
     self._git_exec('rebase')
 
   def _git_exec(self, cmd):
-    execute(f'git {cmd}', cwd=self._assets_dir(), capture_output=False)
+    return execute(f'git {cmd}', cwd=self._assets_dir(), capture_output=False)
 
-  def merge_to_main(self, branch, push_to_remote=False):
+  # tag represent a hash in the filament repo that this merge is associated with
+  def merge_to_main(self, branch, tag, push_to_remote=False):
     self.update()
     assets_dir = self._assets_dir()
+
+    # Update commit message
+    self._git_exec(f'checkout {branch}')
+    code, old_commit = execute(f'git log --format=%B -n 1', cwd=assets_dir)
+    if tag and len(tag) > 0:
+      old_commit += f'\nFILAMENT={tag}'
+      COMMIT_FILE = '/tmp/golden_commit.txt'
+      with open(COMMIT_FILE, 'w') as f:
+        f.write(old_commit)
+      self._git_exec(f'commit --amend -F {COMMIT_FILE}')
+
+    # Do the actual merge
     self._git_exec(f'checkout main')
-    self._git_exec(f'merge --no-ff {branch}')
+    self._git_exec(f'merge --no-ff --no-edit {branch}')
     if push_to_remote and \
        (self.access_token_ or self.access_type_ == ACCESS_TYPE_SSH):
       self._git_exec(f'push origin main')
@@ -109,7 +122,7 @@ def source_from(self, src_dir, commit_msg, branch,
       self._git_exec(f'add {GOLDENS_DIR}')
     else:
       for f in deletes:
-        self._git_exec(f'remove {os.path.join(GOLDENS_DIR, f)}')
+        self._git_exec(f'rm {os.path.join(GOLDENS_DIR, f)}')
       for f in updates:
         shutil.copy2(
           os.path.join(src_dir, f),
diff --git a/test/renderdiff/src/update_golden.py b/test/renderdiff/src/update_golden.py
diff --git a/test/renderdiff/test.sh b/test/renderdiff/test.sh
