Waveform Rendering #2635

crsib · 2022-03-05T09:57:19Z

Resolves: #2531

The goals of this PR are to separate waveform rendering logic into a library and to provide a base for the protentional hardware accelerated approach.

Approach to data caching

Data is cached in blocks with 256 columns each. Each block is identified by a pair of (zoomLevel, firstSample). Each clip has its own set of caches.

Dividing data into blocks helps to avoid unnecessary copying of data, which is expensive on the high-resolution displays. On top of that, we can have hierarchies of caches.

I signed CLA
The title of the pull request describes an issue it addresses
If changes are extensive, then there is a sequence of easily reviewable commits
Each commit's message describes its purpose and effects
There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

Each commit compiles and runs on my machine without known undesirable changes of behavior

Paul-Licameli · 2022-03-05T13:47:48Z

libraries/lib-graphics/CMakeLists.txt

@@ -0,0 +1,30 @@
+#[[
+A library responsible for custom rendering in Audacity
+]]#


The trailing # isn't needed to bracket a long comment

Paul-Licameli · 2022-03-05T13:48:44Z

libraries/lib-graphics/FrameStatistics.cpp

@@ -0,0 +1,130 @@
+/*  SPDX-License-Identifier: GPL-2.0-or-later */


Is this a comment convention we should do in all files?

There should be more regularity but I'm not sure what exactly it should be.

This is a standardized approach, so yes, I would like it see more of it!

Paul-Licameli · 2022-03-05T13:49:15Z

libraries/lib-graphics/FrameStatistics.cpp

@@ -0,0 +1,130 @@
+/*  SPDX-License-Identifier: GPL-2.0-or-later */
+/**********************************************************************


Start with /*! so Doxygen properly sees it

Paul-Licameli · 2022-03-05T13:53:38Z

libraries/lib-graphics/FrameStatistics.h

+
+  FrameStatistics.h
+
+  Dmitry Vedenko


Again /*! and use @file before the file name, and add at least a @brief line about the purpose of the header.

Much more Doxygenation might be added but that much is the minimum I would like so there is some summary on pages like this one https://doxy.audacityteam.org/dir_68267d1309a1af8e8297ef4c3efbcdba.html

I'm less concerned to add @file to .cpp files, because they are not interfaces. It's generally understood that they implement whatever the companion .h file is about.

Paul-Licameli · 2022-03-05T13:55:20Z

src/AudacityApp.cpp

@@ -502,6 +503,7 @@ static void QuitAudacity(bool bForce)
   // termination when exiting is requested
   #if !defined(__WXMAC__)
   LogWindow::Destroy();
+   FrameStatisticsDialog::Destroy();


So the FrameStatisticsDialog is a per-application non-modal dialog (not per-project)?

Paul-Licameli · 2022-03-05T13:56:13Z

src/FrameStatisticsDialog.cpp

@@ -0,0 +1,149 @@
+/*  SPDX-License-Identifier: GPL-2.0-or-later */
+/**********************************************************************


Ditto doxygen

Paul-Licameli · 2022-03-05T13:58:24Z

src/FrameStatisticsDialog.cpp

+   std::chrono::steady_clock::time_point mLastUpdate;
+};
+
+Destroy_ptr<Dialog> sDialog;


I wish to deprecate Destroy_ptr in favor of: https://docs.wxwidgets.org/3.0/classwx_window_ptr_3_01_t_01_4.html

I didn't know of that when I reinvented Destroy_ptr.

Paul-Licameli · 2022-03-05T14:11:32Z

libraries/lib-graphics/PixelSampleMapper.h

+      explicit operator bool() const noexcept;
+   };
+   // GCC 9.3.0 fails horribly if you do not initialize variant explicitly here
+   std::variant<LinearMapper, CustomMapper> mMapper { LinearMapper {} };


The commit that introduces this variant didn't compile for me with clang either. I fixed it for myself by initializing with one argument std::in_place_type<LinearMapper>. I expect the build fix-ups will be squashed with interactive rebasing in a future version of this PR.

I see a commit "Fixes GCC build" adding this line but followed by another that fixes Clang again.

Using in_place_type instead is enough to fix Clang in the first commit, without including SampleCount.h.

If you are curious about why the incomlete type was a problem... I looked a bit at the error-novel for clang and I think there is a metaprogram deducing the correct alternative of the variant to initialize from the constructor arguments, which leads to the expansion of a conditional noexcept specifier, and somehow the operator () of this type is used in an unevaluated context.

But use in_place_type instead and that metaprogram isn't needed to resolve the overloaded constructor.

So, avoiding one more #include inside a header is nice if you can do it, plus there is in-place construction instead of a copy of a LinearMapper. Small gains.

Paul-Licameli · 2022-03-05T14:28:50Z

libraries/lib-utility/MemoryX.h

+
+   const auto result = numerator / denominator;
+
+   return result * denominator == numerator ? result : result + 1;


https://en.cppreference.com/w/cpp/language/operator_arithmetic

If the denominator is zero, there is UB not an exception, so noexcept is strictly speaking correct.

If one value is negative and the other is not, what should happen? (Division must truncate toward zero in C++11 and later; so if this case matters, then it is already "rounded up" and adding 1 is wrong.)

If those cases aren't supposed to happen, add a doxygen @pre that numerator is nonnegative and denominator is positive. (Precondition means "it is the caller's responsibility to exclude those cases, not mine to handle them")

Ignoring the sign question, I would simplify:

return (numerator + denominator - 1) / denominator;

Great catch, thank you!

Your version is faster, but does not consider the possible integer overflow. I think I will just add Unsafe to the function name, though :-)

Paul-Licameli · 2022-03-05T14:38:26Z

libraries/lib-graphics/GraphicsDataCache.h

+   // A multiplier used to control the cache size
+   int32_t mCacheSizeMultiplier { 4 };
+
+   template <typename CacheElementType>


I get a clang build error:

GraphicsDataCache.h:213:23: error: declaration of 'CacheElementType' shadows template parameter template <typename CacheElementType>

Easily fixed just by omitting the template parameter name

Build errors are fixed in later commits. Given that I have to build on 3 different systems I will postpone force pushes to the much later times

Paul-Licameli · 2022-03-05T14:45:05Z

libraries/lib-graphics/CMakeLists.txt

+   PRIVATE
+      lib-math-interface
+      lib-screen-geometry-interface
+      wxwidgets::base


At the later commit that adds more dependencies, I would simplify:

set( LIBRARIES lib-math-interface lib-screen-geometry-interface PRIVATE wxwidgets::base )

The transitive dependency on lib-utility-interface is now implied, so the generated graph of dependencies can have one less arc. Consistently with other libraries, only the dependency on wxBase should be private. Libraries that depend on lib-graphics could themselves simplify their CMakeLists.txt by using transitive dependency on lib-math and lib-screen-geometry.

vsverchinsky · 2022-03-16T15:09:11Z

libraries/lib-graphics/FrameStatistics.cpp

+   if (section < SectionID::Count)
+   {
+      GetInstance().mSections[size_t(section)].AddEvent(duration);
+      GetInstance().mUpdatePublisher.Invoke(section);


does not provide noexcept guarantees

vsverchinsky · 2022-03-18T09:16:04Z

libraries/lib-graphics/GraphicsDataCache.h

+      {
+         if (!mInitializer(key, *element))
+         {
+            DisposeElement(element);


No noexcept guarantees

LWinterberg · 2022-04-29T20:35:39Z

Some quick tests:

"Generate Silence" is really silent now:
The scrub ruler isn't granted space and is overlaying the tracks and z-fighting it
The entire project area is now flickering slightly on my HDR-capable/OLED monitor but not my LCD. Fwiw: This is also the case for Ableton Live, but is not the case for Audacity 3.1.3
The entire thing still seems to be locked to 20Hz

you probably know half of these, but I thought I'd note them down anyway :)

crsib · 2022-05-04T11:49:22Z

The entire thing still seems to be locked to 20Hz

There was an issue with how is selection handled, for example. And there is still lots of stuff to be fixed in that domain.

The entire project area is now flickering slightly

Well, that is something we will have to investigate later. Too bad I have no HDR display available

On-demand blocks were removed in 3.0

crsib requested review from Paul-Licameli and vsverchinsky March 5, 2022 09:57

Paul-Licameli reviewed Mar 5, 2022

View reviewed changes

vsverchinsky reviewed Mar 18, 2022

View reviewed changes

crsib force-pushed the 2531_waveform_rendering branch 3 times, most recently from d2eebd6 to b9f3eab Compare April 22, 2022 16:11

crsib force-pushed the 2531_waveform_rendering branch 2 times, most recently from 1d51e77 to 1b71e23 Compare April 29, 2022 12:19

crsib force-pushed the 2531_waveform_rendering branch from d3fe397 to b79a6f8 Compare May 4, 2022 09:05

crsib force-pushed the 2531_waveform_rendering branch from 84525c4 to fd7e68c Compare May 12, 2022 10:28

crsib added 7 commits May 23, 2022 15:50

Adds basic rendering statistics

73498a1

Limit the update rate of FrameStatisticsDialog

2f02daf

Removes the code related only to on-demand functionality

27f77f5

On-demand blocks were removed in 3.0

Changes the data structure for the Waveform rendering

85039bb

Do not use precalculated where values

e71b490

Adds a templated function that implements integer ceil.

5a601a3

Adds missing macros to the testing frameworks

3c3e0b7

crsib added 29 commits May 31, 2022 19:29

Fixes track label handling

699caca

Fixes clipping on WGL/EGL

35e8e84

Fixes font lookup on macOS

5afb133

Do not draw Ruler offscreen in MeterPanel

63ce0a4

Fixes offscreen rendering outside paint event

fc45720

Do not change GL context state when generating batches

8ab7319

Removes Context::CheckError

7dc73fa

Fixes cache filling from append buffer

e35afee

Removes redundant state change

8c89f22

Fixes batching

d9d2a02

Painter now checks for aabb

5b4d5d4

Fixes size retrieval for rotated text

679869a

Move repaint code to OnIdle instead of OnTimer

3b152fb

Merge branch 'master' into 2531_waveform_rendering

f8af468

Fixes the way Audacity repaints

aee21d4

Log the selected renderer

f1905b4

Attempt to fix Linux packages job

d18fa9f

Fixes mesa name for Fedora

15e5b03

Pin Windows SDK version

181113a

Fixes windows sdk selection

29fc011

Yeat another Linux packaging fix

5bb5917

Use a PowerShell script to run journal tests

9d6ac8a

Verbose logging when replaying journals

838845f

Merge branch 'master' into 2531_waveform_rendering

dd4dd76

Write journal log to a separate file

2cc5ce2

Merge branch 'master' into 2531_waveform_rendering

f00aee9

Fixes build without PCH

edb8b58

Fixes Wayland build when using Fedora with system packages

abed764

Merge branch 'master' into 2531_waveform_rendering

943fd81

crsib closed this Jun 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Waveform Rendering #2635

Waveform Rendering #2635

crsib commented Mar 5, 2022

Paul-Licameli Mar 5, 2022

Paul-Licameli Mar 5, 2022

crsib Mar 5, 2022

Paul-Licameli Mar 5, 2022

Paul-Licameli Mar 5, 2022 •

edited

Paul-Licameli Mar 5, 2022

Paul-Licameli Mar 5, 2022

Paul-Licameli Mar 5, 2022

Paul-Licameli Mar 5, 2022 •

edited

Paul-Licameli Mar 5, 2022

Paul-Licameli Mar 5, 2022 •

edited

crsib Mar 5, 2022

crsib Mar 14, 2022

Paul-Licameli Mar 5, 2022 •

edited

crsib Mar 5, 2022

Paul-Licameli Mar 5, 2022

vsverchinsky Mar 16, 2022

vsverchinsky Mar 18, 2022

LWinterberg commented Apr 29, 2022

crsib commented May 4, 2022

		@@ -0,0 +1,130 @@
		/* SPDX-License-Identifier: GPL-2.0-or-later */

		@@ -0,0 +1,130 @@
		/* SPDX-License-Identifier: GPL-2.0-or-later */
		/**********************************************************************

		@@ -0,0 +1,149 @@
		/* SPDX-License-Identifier: GPL-2.0-or-later */
		/**********************************************************************


		const auto result = numerator / denominator;

		return result * denominator == numerator ? result : result + 1;

Waveform Rendering #2635

Waveform Rendering #2635

Conversation

crsib commented Mar 5, 2022

Approach to data caching

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paul-Licameli Mar 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paul-Licameli Mar 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paul-Licameli Mar 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paul-Licameli Mar 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LWinterberg commented Apr 29, 2022

crsib commented May 4, 2022

Paul-Licameli Mar 5, 2022 •

edited

Paul-Licameli Mar 5, 2022 •

edited

Paul-Licameli Mar 5, 2022 •

edited

Paul-Licameli Mar 5, 2022 •

edited