Audio types / stream properties / cue import#2601
Conversation
|
This is necessary for #2526, so I guess this is a beta-blocker for 2.3.0 as well? |
|
Is this still a work-in-progress? If so, what remains to be done? |
|
@Be-ing WiP because doesn't compile on Xenial and CI is red. Otherwise ready. I don't plan to downgrade the code and won't accept any PRs to do so. |
|
@Holzhaus needs to test if the deferred cue point import enabled by this PR works as expected or if we need to make adjustments. This feature is just an untested bonus of this PR on top of the actual changes. |
uklotzde
changed the title
|
Nice work! I think we should gradually migrate all code that calculates times from samples/frames to use this new SignalInfo class, both in the audio code and GUI code. There are lots of places in the GUI code that relies on ControlProxies to get that info. It would be better to get the SignalInfo from the Track/Cue/whatever object. Then we could get away from using samples as units of time. |
|
@mixxxdj/developers needs review |
|
I'm hitting a |
|
@Holzhaus An invalid debug assertion was only triggered by SoundSourceMP3. I have removed the wrong debug assertion and pulled the other one up into the base class. It is now checked for all SoundSources, independent of the format. |
| public: TYPE const& get##CAP_NAME() const { return m_##NAME; } \ | ||
| public: TYPE& ref##CAP_NAME() { return m_##NAME; } \ | ||
| public: constexpr TYPE const& get##CAP_NAME() const { return m_##NAME; } \ | ||
| public: constexpr TYPE& ref##CAP_NAME() { return m_##NAME; } \ |
There was a problem hiding this comment.
This allows to modify the m_##NAME via a non-const reference which we like to avoid in Mixxx.
Is this actually used? Can we remove this, or replace with
public: constexpr TYPE* get##CAP_NAME() { return m_##NAME; }
There was a problem hiding this comment.
This is not a by-ref parameter. Yes, the mutable access is needed for efficient move-assignement at some places. The function name allows to find all occurrences in contrast to accessing a public member directly.
There was a problem hiding this comment.
A getter with mutable access? No. And I am not going to rewrite many other parts of the code.
Btw, the code documentation explicitly states why the mutable reference is needed ;)
There was a problem hiding this comment.
Ok, so we gen call it getPtr##NAME or similar. I think this has slipped through in earlier PRs, so I think this is not necessary to fix it.
Maybe we can add the pointer version and use it for new code.
There was a problem hiding this comment.
I don't understand the mutable reference part by the way.
It is finally a sytax issue only.
There was a problem hiding this comment.
Otherwise you need to forbid implementing indexing operators with their common syntax.
There was a problem hiding this comment.
For deeply nested structures as documented. I refuse to do a mass-rewrite of all existing code.
I have already acknowledged the a mass re-write is not necessary.
But if I look to the using code I find cases where the non-const reference is used as local variable or where it is converted into a pointer. At least for those cases a pointer returning function is necessary to write code without non-const references. This allows to avoid the same confusing cases we avoid by the non-const reference parameter rule, writing external memory via the local variable syntax.
There was a problem hiding this comment.
These accessor functions are not intended for storing the returned reference. Only to assign a new value.
There was a problem hiding this comment.
...or to modify the existing value in-place.
There was a problem hiding this comment.
Unfortunately it is used for storing. I would like to fix at least these places because this is a source of confusion. I don't mind about this, where the refrence is not stored:
m_record.refMetadata().refTrackInfo().setBpm(mixxx::Bpm(bpmValue));
vs:
m_record.ptrMetadata()->ptrTrackInfo()->setBpm(mixxx::Bpm(bpmValue));
@daschuer No agreement about if returning a mutable reference is permitted or not. Returning a pointer is "pointless" and may only tempt the caller to store somewhere that my outlive the current scope. We don't need to write C code for this special use case. I won't.
Looking from the machine code, storing a reference or a pointer is the same, so that's not the point. It is a code style issue and readability issue.
Except for STL compatibility of container classes QT never returns non canst references.
Since this is a common macro, we should at least allow users to do the same.
Qt Example:
controlsTable->verticalHeader()->setVisible(false);
IMHO we should at least add a ptr##NAME function or just a ##NAME and using it at all places where we currently store a non-const deference.
What do you think?
If you like I can issue a PR against your branch.
|
A restrictive implementation could be extended when needed, but not now! We seem to have different opinions on how to develop safe and maintainable software. |
|
I think we could benefit from a larger discussion of a plan to move away from using stereo samples as units of time. That's a big task that this PR won't solve. I'm okay with merging this without those conversion functions, but the problem remains that we're already doing those conversions all over the code. See WOverview::samplePositionToSeconds and how much it used throughout WOverview because cue point positions are exposed to the GUI as stereo samples. |
If the object lives on the stack or In the heap is a not relevant implementation detail. We are discussing here an universal macro. It should be usable independent from how the object is allocated. The main issue I like to solve is to get rid of the non-const references in the code. Would it be OK for you if I do a PR to fix this? |
|
Any other opinions? |
|
There is no reason to hold this PR back because of this non-const reference issue. |
I understood the you wanted to replace all |
|
I don't really care if we use pointer or references, but I often feel uncomfortable when using pointers without checking if they are null first, which would make the code less straightforward here. |
I'd suggest to use function names with |
I guess it is only a single function right now. It is a good idea, because there are already many existing |
|
macOS builds on the build server are failing: Do we need to update the compiler on the macOS build server? We may need to anyway to fix https://bugs.launchpad.net/mixxx/+bug/1871238 |
|
These It is an inconvenient situation if the version on the build server is different than the CI. As a developer I have to rely on the outcomes of the CI builds. This is the only feedback loop that is available. |
|
Am I correct in thinking that error is probably because of an outdated compiler? |
|
Yes. A similar situation as with Xenial. The compiler seems to only partially support C++17 features. |
|
I'm even unsure which minimum version would be required: |
|
If we go through the trouble to update, we should update to the latest version. |
|
I have already a solution that is not too hacky. I used it during the review here. The issue is that it is required for a constexpr class to have all functions constantexpr as well. From c++ 17 returning a pointer and reference from a class is also considered as contstexpr. This is relevant vor QString(), be cause class with a QString() can not be a constexpr because if the non constexpr destructor. A constexpr function as IMHO no value. If it is in the header the Compiler will inline them anyway. It can only be missleading for the developer. |
|
I am not sure if the conditional include directives in util/optional.h include <experimental/optional> instead of on macOS and why? On macOS should be available since at least Clang 5! |
|
Clang 5 has full C++17 support. The experimental headers are only provided for backwards compatibility. We should first try to remove the experimental hack from util/optional.h before introducing the next workaround. |
|
We're using Xcode 9.4.1 which has clang 9.1.0. I can update us to Xcode 10.1 (clang 10.0.0 / llvm 6.0.1) without upgrading the OS. Though I'm confused because as @uklotzde says, c++17 should be somewhat solid on this compiler. Are we sure it's isn't a bug in our workarounds? |
5 participants
AudioSource, i.e. remove the ugly, Janus-headedAudioSignalbase classCueInfointoTrackobjects, needed for Read Cues from Serato MP3 Tags #2526. I have added this here as a proof of concept that the refactoring achieves the intended goal. Needs to be tested when used!