Realtime Compositor: Implement Movie Distortion node #108230

Merged
Omar Emara merged 3 commits from OmarEmaraDev/blender:compositor-movie-distortion into main 2023-06-07 14:45:53 +02:00
Member

This patch implements the Movie Distortion node for the realtime
compositor. The distorted coordinates are computed and cached for a
particular tracking camera distortion parameters. So for expensive
distortion models, the first run will take some time to compute, but
subsequent runs will be fast.

An alternative implementation would be to implement each of the
distortion modes in the shader, but that was decided against for a few
reasons:

  1. We want to hide the implementation details of the distortion models,
    since it is provided through an external library (Libmv).
  2. Some distortion models are expensive to solve accurately, and can be
    quite slow to solve each time the shader runs.
  3. The typical usage of the node does not involve interactive editing of
    the distortion parameters, rather, the parameters are computed during
    camera calibration, so caching seems most fitting in that case.
This patch implements the Movie Distortion node for the realtime compositor. The distorted coordinates are computed and cached for a particular tracking camera distortion parameters. So for expensive distortion models, the first run will take some time to compute, but subsequent runs will be fast. An alternative implementation would be to implement each of the distortion modes in the shader, but that was decided against for a few reasons: 1. We want to hide the implementation details of the distortion models, since it is provided through an external library (Libmv). 2. Some distortion models are expensive to solve accurately, and can be quite slow to solve each time the shader runs. 3. The typical usage of the node does not involve interactive editing of the distortion parameters, rather, the parameters are computed during camera calibration, so caching seems most fitting in that case.
Omar Emara added the
Interest
Compositing
Module
EEVEE & Viewport
labels 2023-05-24 15:03:06 +02:00
Omar Emara added 1 commit 2023-05-24 15:03:16 +02:00
0d390140eb Realtime Compositor: Implement Movie Distortion node
This patch implements the Movie Distortion node for the realtime
compositor. The distorted coordinates are computed and cached for a
particular tracking camera distortion parameters. So for expensive
distortion models, the first run will take some time to compute, but
subsequent runs will be fast.

An alternative implementation would be to implement each of the
distortion modes in the shader, but that was decided against for a few
reasons:

1. We want to hide the implementation details of the distortion models,
   since it is provided through an external library (Libmv).
2. Some distortion models are expensive to solve accurately, and can be
   quite slow to solve each time the shader runs.
3. The typical usage of the node does not involve interactive editing of
   the distortion parameters, rather, the parameters are computed during
   camera calibration, so caching seems most fitting in that case.
Omar Emara requested review from Clément Foucault 2023-05-24 15:03:46 +02:00
Clément Foucault requested review from Sergey Sharybin 2023-05-27 18:52:29 +02:00
Clément Foucault approved these changes 2023-05-27 18:56:14 +02:00
Clément Foucault left a comment
Member

This looks good to me. I think the approach is good as the tracking distortion doesn't usually change when previewing animation which is the primary goal.

I just added Sergey as reviewer so that he knows about the addition inside the tracking module. But thoses looks trivial.

This looks good to me. I think the approach is good as the tracking distortion doesn't usually change when previewing animation which is the primary goal. I just added Sergey as reviewer so that he knows about the addition inside the tracking module. But thoses looks trivial.
Sergey Sharybin requested changes 2023-05-30 10:55:11 +02:00
Sergey Sharybin left a comment
Owner

Generally, it is probably indeed the best way of moving forward.

Thing is. Some of the implementation feels unideal and makes things like adding more distortion models even more complicated. Unfortunately, I can not think of an easy change to avoid this increased technical debt which will be good long-term. Part of the reason is that avoiding C-API and using things like CameraIntrinsics, PackedIntrinsics etc is the ultimate way avoiding duplication, but we'll need a bigger refactor for that first.

So for moving forward with the compositor we probably have to accept this technical debt, and solve it separately. There are few things which we need to do still though:

  • Rename BKE_tracking_camera_equal to BKE_tracking_camera_distortion_equal
  • Similar to the hashing of the distortion model.
  • Add a comment saying "Compare camera parameters which are related on the distortion", and also add a note that the functionality will change in the future and that the function needs to be used with caution about it.

Another thing I am wondering is the life time of the cached distortion grid. It is not very clear from the code when the memory is freed. Can you elaborate a bit about that?

Generally, it is probably indeed the best way of moving forward. Thing is. Some of the implementation feels unideal and makes things like adding more distortion models even more complicated. Unfortunately, I can not think of an easy change to avoid this increased technical debt which will be good long-term. Part of the reason is that avoiding C-API and using things like `CameraIntrinsics`, `PackedIntrinsics` etc is the ultimate way avoiding duplication, but we'll need a bigger refactor for that first. So for moving forward with the compositor we probably have to accept this technical debt, and solve it separately. There are few things which we need to do still though: - Rename `BKE_tracking_camera_equal ` to `BKE_tracking_camera_distortion_equal ` - Similar to the hashing of the distortion model. - Add a comment saying "Compare camera parameters which are related on the distortion", and also add a note that the functionality will change in the future and that the function needs to be used with caution about it. Another thing I am wondering is the life time of the cached distortion grid. It is not very clear from the code when the memory is freed. Can you elaborate a bit about that?
@ -2198,6 +2202,62 @@ void BKE_tracking_camera_principal_point_pixel_set(struct MovieClip *clip,
principal_point_pixel, frame_width, frame_height, camera->principal_point);
}
bool BKE_tracking_camera_equal(const MovieTrackingCamera *a, const MovieTrackingCamera *b)

This is a bit confusing. Camera also defines sensor width, which is not compared here.

This is a bit confusing. Camera also defines sensor width, which is not compared here.
OmarEmaraDev marked this conversation as resolved
@ -2201,0 +2225,4 @@
return a->brown_k1 == b->brown_k1 && a->brown_k2 == b->brown_k2 &&
a->brown_k3 == b->brown_k3 && a->brown_k4 == b->brown_k4 &&
a->brown_p1 == b->brown_p1 && a->brown_p2 == b->brown_p2;
default:

Please do not add default unless absolutely needed, as it makes it very hard to see at compile time where enumerator value is forgotten to be checked when adding a new value.

In this example it is as easy as moving the BLI_assert_unreachable past the switch statement.

Please do not add `default` unless absolutely needed, as it makes it very hard to see at compile time where enumerator value is forgotten to be checked when adding a new value. In this example it is as easy as moving the `BLI_assert_unreachable` past the `switch` statement.
OmarEmaraDev marked this conversation as resolved
@ -2201,0 +2234,4 @@
uint64_t BKE_tracking_camera_hash(const MovieTrackingCamera *camera)
{
using namespace blender;
switch (camera->distortion_model) {

Is it intended to skip distortion_model, focal and so on?

Is it intended to skip `distortion_model`, `focal` and so on?
OmarEmaraDev marked this conversation as resolved
@ -2201,0 +2253,4 @@
float2(camera->principal_point),
float4(camera->brown_k1 && camera->brown_k2 && camera->brown_k3 && camera->brown_k4),
float2(camera->brown_p1, camera->brown_p2));
default:

Same as above.

Same as above.
OmarEmaraDev marked this conversation as resolved
@ -0,0 +1,84 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */

Please add copyright to the new files.

Please add copyright to the new files.
OmarEmaraDev marked this conversation as resolved
Author
Member

Another thing I am wondering is the life time of the cached distortion grid. It is not very clear from the code when the memory is freed. Can you elaborate a bit about that?

@Sergey This is documented in the header files of StaticCacheManager, CachedResources, and CachedResourceContainer classes. Start from here: https://projects.blender.org/blender/blender/src/branch/main/source/blender/compositor/realtime_compositor/COM_static_cache_manager.hh#L17

> Another thing I am wondering is the life time of the cached distortion grid. It is not very clear from the code when the memory is freed. Can you elaborate a bit about that? @Sergey This is documented in the header files of StaticCacheManager, CachedResources, and CachedResourceContainer classes. Start from here: https://projects.blender.org/blender/blender/src/branch/main/source/blender/compositor/realtime_compositor/COM_static_cache_manager.hh#L17
Omar Emara added 2 commits 2023-06-06 16:41:52 +02:00
Sergey Sharybin approved these changes 2023-06-07 10:57:24 +02:00
Sergey Sharybin left a comment
Owner

Thanks for the update and pointer to the docs.

Lets move of with this patch, and then I'll take care of some code de-duplication in the main branch from the motion tracking module :)

Thanks for the update and pointer to the docs. Lets move of with this patch, and then I'll take care of some code de-duplication in the main branch from the motion tracking module :)
Omar Emara merged commit f06929766b into main 2023-06-07 14:45:53 +02:00
Omar Emara deleted branch compositor-movie-distortion 2023-06-07 14:45:54 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#108230
No description provided.