VSE: Add sound strip retiming support #105072

Merged
Richard Antalik merged 16 commits from iss/blender:retiming-sound-2 into main 2023-04-21 16:53:39 +02:00

This patch contains changes needed for retiming sound strips.

BKE_sound_set_scene_sound_pitch() is replaced by
BKE_sound_set_scene_sound_pitch_constant_range() which uses new
Audaspace interface to set pitch in bulk.
This is done in SEQ_retiming_sound_animation_data_set() where retimed
sections are created for each strip. When strip is inside of meta
strip(s), the retimed sections of meta and actual strip are split where
they intersect and pitch is multiplied where they overlap. Each section
will have pitch value that is provided to audaspace.

Waveform overlay now represents retimed audio accurately.

Ref: #100337


Versioning for animated pitch is not present, because resulting data is unusable (whole animation is baked, so changing retiming is basically impossible). It would also result in precision issues which results in non uniform sound pitch in some cases.

Changes in audaspace are meged to upstram.

This patch contains changes needed for retiming sound strips. `BKE_sound_set_scene_sound_pitch()` is replaced by `BKE_sound_set_scene_sound_pitch_constant_range()` which uses new Audaspace interface to set pitch in bulk. This is done in `SEQ_retiming_sound_animation_data_set()` where retimed sections are created for each strip. When strip is inside of meta strip(s), the retimed sections of meta and actual strip are split where they intersect and pitch is multiplied where they overlap. Each section will have pitch value that is provided to audaspace. Waveform overlay now represents retimed audio accurately. Ref: #100337 ---- Versioning for animated pitch is not present, because resulting data is unusable (whole animation is baked, so changing retiming is basically impossible). It would also result in precision issues which results in non uniform sound pitch in some cases. Changes in audaspace are meged to upstram.
Richard Antalik added 1 commit 2023-02-22 13:58:40 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
30b56abb01
VSE: Add sound strip retiming support
This patch contains changes needed for retiming sound strips. This is a part of blender/blender#104544

----

Deviation from blender/blender#100337 is, that changes in audaspace are not separated.
Versioning for old files is not present, because resulting data is unusable (whole animation is baked, so changing retiming is basically impossible). Versioning also results in precision issues which results in non uniform sound pitch in some cases. And finally calculating strip offset correction is non-trivial (that is least of an issue, but worth mentioning still). So I would advocate to not convert old data to current implementation. I think, that supporting smooth speed transitions would allow for more efficient versioning code.
Richard Antalik requested review from Sergey Sharybin 2023-02-22 14:04:12 +01:00
Richard Antalik requested review from Joerg Mueller 2023-02-22 14:05:52 +01:00

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR105072) when ready.
Sergey Sharybin added this to the Video Sequencer project 2023-02-28 15:43:57 +01:00
Sergey Sharybin added the
Module
VFX & Video
Interest
Video Sequencer
labels 2023-02-28 15:44:06 +01:00
Joerg Mueller requested changes 2023-03-17 19:05:28 +01:00
Joerg Mueller left a comment
Member

Hey, sorry for taking some time to review this, I've been quite busy recently. I have looked over the code changes now, primarily in audaspace and added corresponding comments. I also briefly looked over the Blender changes, which look fine to the extent that I can judge them, maybe @Sergey wants to take a closer look there.

Hey, sorry for taking some time to review this, I've been quite busy recently. I have looked over the code changes now, primarily in audaspace and added corresponding comments. I also briefly looked over the Blender changes, which look fine to the extent that I can judge them, maybe @Sergey wants to take a closer look there.
@ -167,1 +167,4 @@
AUD_API void AUD_SequenceEntry_setAnimationData_constant_range(AUD_SequenceEntry* entry, AUD_AnimateablePropertyType type, int frame_start, int frame_end, float* data)
{
AnimateableProperty* prop = (*entry)->getAnimProperty(static_cast<AnimateablePropertyType>(type));
Member

Audaspace has its own coding style as you surely noticed, since you're following most of it. Please make sure that indentation happens with tabs everywhere. I just recently introduced a clang-format file in upstream that you can find here: https://github.com/audaspace/audaspace/blob/master/.clang-format. Please only apply it to the changes though, since not all of audaspace is following it perfectly and I wouldn't want to make this patch verbose because of it (see the clang-format-diff script or https://offlinemark.com/2021/04/02/surgical-formatting-with-git-clang-format/ for example).

While we're at it: I would appreciate if you also create a pull request for the audaspace changes upstream (https://github.com/audaspace/audaspace/pulls)!

Audaspace has its own coding style as you surely noticed, since you're following most of it. Please make sure that indentation happens with tabs everywhere. I just recently introduced a clang-format file in upstream that you can find here: https://github.com/audaspace/audaspace/blob/master/.clang-format. Please only apply it to the changes though, since not all of audaspace is following it perfectly and I wouldn't want to make this patch verbose because of it (see the clang-format-diff script or https://offlinemark.com/2021/04/02/surgical-formatting-with-git-clang-format/ for example). While we're at it: I would appreciate if you also create a pull request for the audaspace changes upstream (https://github.com/audaspace/audaspace/pulls)!
iss marked this conversation as resolved
@ -115,0 +118,4 @@
* \param position_start The start position in the animation in frames.
* \param position_end The end position in the animation in frames.
*/
void write_range(const float* data, int position_start, int position_end);
Member

Please rename write_range similarly to the other API functions that call it, so that its also clear from the name that it writes the same data into the range and doesn't get the full data for the range in the parameters. I.e. it repeats the data found in the parameter rather than expecting a bigger block of data that may differ all the way from start to end.

Please rename `write_range` similarly to the other API functions that call it, so that its also clear from the name that it writes the same data into the range and doesn't get the full data for the range in the parameters. I.e. it repeats the data found in the parameter rather than expecting a bigger block of data that may differ all the way from start to end.
iss marked this conversation as resolved
@ -64,2 +64,4 @@
double m_skip;
/// The FPS of the scene.
float m_fps;
Member

Likely a bug: Rather than having the fps duplicated here, please add a std::shared_ptr<SequenceData> and get the fps from there. This way, it will be updated when Sequence::setFPS is called which happens when the FPS in blender are changed!

Likely a bug: Rather than having the fps duplicated here, please add a `std::shared_ptr<SequenceData>` and get the fps from there. This way, it will be updated when `Sequence::setFPS` is called which happens when the FPS in blender are changed!
iss marked this conversation as resolved
@ -245,3 +244,1 @@
if(seekpos < 0)
seekpos = 0;
seekpos += m_entry->m_skip;
float seek_frame = (position - m_entry->m_begin) * m_entry->m_fps;
Member

Is there a specific reason you are using floats here (seek_frame and target_frame) now rather than doubles (as was used for seekpos before)? I think it's still critical for accuracy of long sequences (e.g. full streams of movies at something like 90 minutes).

Is there a specific reason you are using floats here (`seek_frame` and `target_frame`) now rather than doubles (as was used for `seekpos` before)? I think it's still critical for accuracy of long sequences (e.g. full streams of movies at something like 90 minutes).
iss marked this conversation as resolved
@ -248,0 +246,4 @@
seek_frame = 0;
seek_frame += m_entry->m_skip * m_entry->m_fps;
AnimateableProperty* pitch_property = m_entry->getAnimProperty(AP_PITCH);
Member

Just a note: since this only considers the AP_PITCH property and not the doppler effect happening with 3D audio, this will only seek correctly for pitch animation not for doppler animated sounds. But that's ok, considering the 3D audio case is even more complicated and expensive to compute. So in Blender VSE sequences work better then, but 3D speakers don't.

Just a note: since this only considers the `AP_PITCH` property and not the doppler effect happening with 3D audio, this will only seek correctly for pitch animation not for doppler animated sounds. But that's ok, considering the 3D audio case is even more complicated and expensive to compute. So in Blender VSE sequences work better then, but 3D speakers don't.
iss marked this conversation as resolved
@ -248,0 +251,4 @@
float target_frame = 0;
if(pitch_property != nullptr)
{
for(int i = 0; i < seek_frame; i++)
Member

Since this is just an integer loop, the fractal part of seek_frame is not considered. For more accurate seeking, I recommend to still consider it though.

Since this is just an integer loop, the fractal part of `seek_frame` is not considered. For more accurate seeking, I recommend to still consider it though.
iss marked this conversation as resolved
Richard Antalik added 2 commits 2023-03-21 18:44:07 +01:00
Richard Antalik added 2 commits 2023-04-04 23:14:46 +02:00
Richard Antalik added 1 commit 2023-04-05 01:29:23 +02:00
Richard Antalik added 1 commit 2023-04-05 19:33:49 +02:00
78d8b02a15 Fix use after free.
Still don't quite understand how std::shared_ptr is supposed to be used, but I think this is corect way.
Author
Member

@neXyon I have marked all issues as resolved, not sure if my inline responses were submitted - seems they are not, so will ask here:

Shall we continue with review here and then I submit PR to audaspace repo?
You wanted to rename write_range, but wasn't very specific, so I assumed you meant write_constant_range.

@neXyon I have marked all issues as resolved, not sure if my inline responses were submitted - seems they are not, so will ask here: Shall we continue with review here and then I submit PR to audaspace repo? You wanted to rename `write_range`, but wasn't very specific, so I assumed you meant `write_constant_range`.
Richard Antalik added 1 commit 2023-04-05 23:55:41 +02:00
Joerg Mueller approved these changes 2023-04-06 16:36:54 +02:00
Joerg Mueller left a comment
Member

I've added two name change suggestions, just fixing their styling.
I think the audaspace changes are ready, so there won't be much to continue reviewing, when you create a PR upstream :)

I've added two name change suggestions, just fixing their styling. I think the audaspace changes are ready, so there won't be much to continue reviewing, when you create a PR upstream :)
@ -71,0 +72,4 @@
* \param frame_end End of the frame range.
* \param data The data to write.
*/
AUD_API void AUD_SequenceEntry_setAnimationData_constant_range(AUD_SequenceEntry* entry, AUD_AnimateablePropertyType type, int frame_start, int frame_end, float* data);
Member

AUD_SequenceEntry_setConstantRangeAnimationData

`AUD_SequenceEntry_setConstantRangeAnimationData`
neXyon marked this conversation as resolved
@ -115,0 +118,4 @@
* \param position_start The start position in the animation in frames.
* \param position_end The end position in the animation in frames.
*/
void write_constant_range(const float* data, int position_start, int position_end);
Member

writeConstantRange

`writeConstantRange`
neXyon marked this conversation as resolved
Richard Antalik added 1 commit 2023-04-06 22:27:57 +02:00
Richard Antalik added 1 commit 2023-04-06 23:33:52 +02:00
Richard Antalik added 1 commit 2023-04-07 00:14:58 +02:00
Richard Antalik added 1 commit 2023-04-17 18:31:28 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
42483e70b6
Merge branch 'main' into retiming-sound-2
Sergey Sharybin reviewed 2023-04-18 10:00:00 +02:00
Sergey Sharybin left a comment
Owner

Patch seems nice and small :)
Some cosmetic feedback for now.

Patch seems nice and small :) Some cosmetic feedback for now.
@ -248,0 +283,4 @@
class RetimingRangeData {
public:
std::vector<RetimingRange> ranges;

Any specific reason to use std::vector instead of blender::Vector?
Unless there is really goo reason (like, interfacing with an external library) we'd better be consistently using our own primitives.

Any specific reason to use `std::vector` instead of `blender::Vector`? Unless there is really goo reason (like, interfacing with an external library) we'd better be consistently using our own primitives.
iss marked this conversation as resolved
@ -248,0 +303,4 @@
RetimingRangeData &operator*=(const RetimingRangeData rhs)
{
if (ranges.size() == 0) {

if (!ranges.empty()) (is_empty for blender C++ primitives).

Also, consider early output, to unindent the not-so-trivial-logic below.

`if (!ranges.empty())` (`is_empty` for blender C++ primitives). Also, consider early output, to unindent the not-so-trivial-logic below.
iss marked this conversation as resolved

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR105072) when ready.
Richard Antalik added 1 commit 2023-04-19 16:00:16 +02:00
Richard Antalik added 1 commit 2023-04-19 16:31:39 +02:00

Something seems odd in the current buildbot build:

  • it does not seem possible to make the entire audio track faster by dragging it's right handle
  • adding and moving handles in the middle strip does not update the strip length
Something seems odd in the current buildbot build: - it does not seem possible to make the entire audio track faster by dragging it's right handle - adding and moving handles in the middle strip does not update the strip length
Richard Antalik added 1 commit 2023-04-19 18:30:34 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
89512d7128
Fix sound strip length not changing

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR105072) when ready.
Author
Member

Something seems odd in the current buildbot build:

  • it does not seem possible to make the entire audio track faster by dragging it's right handle
  • adding and moving handles in the middle strip does not update the strip length

Oops, forgot to include this fix. I assume I fixed both issues, but if not please clarify how to reproduce first one.

> Something seems odd in the current buildbot build: > > - it does not seem possible to make the entire audio track faster by dragging it's right handle > - adding and moving handles in the middle strip does not update the strip length Oops, forgot to include this fix. I assume I fixed both issues, but if not please clarify how to reproduce first one.
Sergey Sharybin approved these changes 2023-04-20 09:49:53 +02:00
Sergey Sharybin left a comment
Owner

Not sure what's going on with those failed regression tests in the build, probably just branch needs to be updated to the latest main state.

From my testing the retiming works as expected now.

Not sure what's going on with those failed regression tests in the build, probably just branch needs to be updated to the latest main state. From my testing the retiming works as expected now.
Richard Antalik added 1 commit 2023-04-21 16:29:47 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
908cb5cc25
Merge branch 'main' into retiming-sound-2
Author
Member

@blender-bot build

@blender-bot build
Richard Antalik merged commit b21695a507 into main 2023-04-21 16:53:39 +02:00
Richard Antalik deleted branch retiming-sound-2 2023-04-21 16:53:40 +02:00
Sergey Sharybin removed this from the Video Sequencer project 2023-11-22 11:56:24 +01: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
4 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#105072
No description provided.