Audaspace: Load CoreAudio device on demand #107607
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#107607
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "iss/blender:core-audio-on-demand"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Refactor CoreAudioDevice code to support on demand device opening and
closing when sound is not playing.
Currently device is opened when AUD_init() is called and then closed
immediately. This is because it seems, that this is causing issues with
synchronizer and causes AV sync to fail.
Anothe issue seems to be usage of audio scrubbing. This seems to confuse the device and it fails to play audio at all after some time. For WASAPI this was also problematic and a solution could be to create a timer in new thread, that would delay closing of the device.
@blender-bot package
Package build started. Download here when ready.
Please follow the code style of audaspace and do not apply Blender's clang format in this file. This leads to way too many changes in this file. Audaspace has it's own clang format changes, but you should only format actually changed code.
I'd also name the methods simply
open
andclose
, the Device is superfluous and the open function is not on demand, it just opens.Apart from that, the changes look good as far as I can tell. Cannot try it, since I lack the hardware though.
@neXyon Hi, this should be ready to review,.Can you have a look?
Please fix the formatting first as I mentioned before, otherwise it's hard to read the diff.
@neXyon Sorry I was pretty sure I did that, but I was wrong. Would you in future accept plain cleanup patch to format the file? Otherwise I have to undo unintended changes manually which is bit tedious.
Not sure what you mean? Do you mean running clang-format as auto-formatter? Clang format can also be run on just the changes in a diff which would be ideal here. So, can you apply the cleanup patch here already?
I meant to make the files conform to format rules before I make any changes to files. I don't have IDE setup to work on Audaspace code, and MSVC is ignoring clang format currently anyway. Not sure if I broke the addon somehow. It's just in Blender we like to keep our code formatted at all times, so I thought about doing same thing in this case.
OMG I forgot to commit changes before pushing :D
Overall changes look good now! I've added some comments inline of what I'd like to see changed still and additionally:
The threaded functionality is very similar to what
ThreadedDevice
is doing so it is a bit of code duplication if I did not miss anything and that code can be reused (m_device_opened
andm_delayed_close_finished
correspond tom_playing
,m_stop
in some sense andm_delayed_close_thread
corresponds tom_thread
ofc). Additionally, the automatic opening and closing could be used by other backends as well. Therefore, please create a classOpenCloseDevice
or so, derived fromThreadedDevice
that adds abstract/purely virtual methods foropen
/close
and does all the functionality that can be shared, i.e., implementingThreadedDevice
'srunMixingThread
with whatcloseAfterDelay
does.@ -170,0 +196,4 @@
{
for(;;)
{
std::this_thread::sleep_for(std::chrono::microseconds(5));
I dislike the hardcoding of this value, especially since it reappears just a few lines below again. Maybe you could turn this into a proper variable and even base it on some other value that makes sense (see e.g.
sleep_duration
inWASAPIDevice.cpp:95
).Sure, makes sense to use variables here, but not sure if I can base this on something else. My goal was to have multiple seconds until device is closed. If these delays would not be constant, some users may have issues and some not, or they would have trouble providing reproducible case.
@ -170,0 +198,4 @@
{
std::this_thread::sleep_for(std::chrono::microseconds(5));
if(m_playback || m_playback_stopped_time == 0)
time(&m_playback_stopped_time);
Please don't mix
std::chrono
code withctime
code, just usestd::chrono
everywhere!@ -29,7 +29,9 @@
#include "CoreAudioSynchronizer.h"
#include "devices/SoftwareDevice.h"
#include <ctime>
Please use
chrono
instead ofctime
.@iss what is the status of this patch? Can this be wrapped up?
@fsiddi Will prioritize this. I focused most of my efforts on retiming and bugfixing, so did revisit this when I got time to spare.
Sorry for delay, I have allocated some time to finish this now. Also sorry if I am plain wrong here, I have still limited understanding of cpp abstraction in this case.
OpenCloseDevice
would have to have alsostart
andstop
virtual functions, since it needs to useplaying
function to control when the device should be opened and closed.Then looking at
ThreadedDevice
this also implements playing function, but it would have to be overridden, which I can do. In that case, I would be able to reuseThreadedDevice::start()
function, but to me it seems I would have to do quite a bit of gymnastics to make it work. To me this solution looks highly confusing, and I would propose to derive fromSoftwareDevice
class. The threading that is used is super simple anyway.If I am wrong, I would still probably do that as I think it is better starting point for code optimization.
b5b5b95fda
to8f65025a11
@neXyon Do you have policy on how copyright blocks should be updated?
I have thought a bit more about implications of previous message, and currently I think there would be some issues if you needed a class derived from both
OpenCloseDevice
andThreadedDevice
Sorry for taking some time to respond, I'm as busy as usual. The patch looks good and you're right, the code probably doesn't get much better if you derive from
ThreadedDevice
. I've made minor fixes (attached), please add those and then I think this is ready to merge. It would be great if you could also open a PR for this at upstream audaspace (https://github.com/audaspace/audaspace/) as mentioned before. And no I don't have a policy for the copyright blocks. You can put your name and current dates if you wish :)Thanks for checking, will apply your changes and post PR in audaspace repo. As far as copyright goes, I will then just update year. I don't believe in intellectual property anyway :D
Created PR for upstream, closign this one.
Hmm fairly consistently getting this crash on Mac when quitting Blender, after scrubbing something in video sequencer that has audio clips.
Pull request closed