Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Waveform stops rendering when DVS is CUEed #10764

Closed
mixxxbot opened this issue Aug 23, 2022 · 24 comments
Closed

Waveform stops rendering when DVS is CUEed #10764

mixxxbot opened this issue Aug 23, 2022 · 24 comments

Comments

@mixxxbot
Copy link
Collaborator

Reported by: nm2107
Date: 2022-06-29T19:06:03Z
Status: New
Importance: Undecided
Launchpad Issue: lp1980264
Tags: passthrough, waveform


Hello,

Bug appears on Mixxx 2.3.3 (I installed it via flatpak, but I suspect all the other distributions ways are affected).

Steps to reproduce :

  • Load a track on a DVS deck.
  • Play the track and set multiple hotcues.
  • Press the CUE button : the playback stops and the cursor position goes back to the CUE point. All good so far.
  • Press any hotcues previously set : the cursor moves to the hotcue, but the waveform is not updated (i.e. the cursor is still displayed on the CUE position due to the lack of waveform rendering update, but the cursor position is actually on the pressed hotcue).
    Press play : the waveform is rendered as usual with the cursor position starting from the pressed hotcue (feels like a jump due to the lack of rendering in the previous step).

Mixxx 2.3.2 was fine, I suspect #4789 and #4791 to be the cause of the issue. Why in these PRs there is no check about passthrough mode if the passthrough was the issue (i.e. like #4787 has attempted to do) ?

Note : I'm not using the passthrough mode.

Thank you for your help :)

@mixxxbot
Copy link
Collaborator Author

Commented by: nm2107
Date: 2022-06-29T19:16:24Z


Additionally, when loading a track on a deck where there was already a track (which is stopped), the waveform of the new track is displayed as empty, until the track is played.

@mixxxbot
Copy link
Collaborator Author

Commented by: nm2107
Date: 2022-06-30T19:36:22Z


Just to confirm that commenting the return statement in this code block fixes my issue :

https://github.com/mixxxdj/mixxx/blob/2.3.3/src/engine/enginebuffer.cpp#L1260-L1270

However, commenting only the m_filepos_play == kInitialSamplePosition part of the condition doesn't fixes the issue.

I understand the m_trackSampleRateOld == 0 and m_tempo_ratio_old == 0 checks to prevent division by 0 (operated a few lines later), but why is the m_filepos_play == kInitialSamplePosition check for ? And also, why am I entering the condition body ? Only because the DVS signal is quiet (i.e. I am on a CUE point) ? Shouldn't we perform an other check to determine whether we're playing instead of relying on the tempo_ratio ?

@mixxxbot
Copy link
Collaborator Author

Commented by: nm2107
Date: 2022-06-30T19:55:41Z


It seems that the proposed fix (that actually introduced my issue), was attempting to fix an OOM about beat painting in waveform rendering (c.f. https://bugs.launchpad.net/mixxx/+bug/1977662/comments/3 ).

In that case, why the proposed fix has introduced the condition pointed out in my previous comment, instead of dealing with the OOM where it happens ? (i.e. make a check in the waveform beat rendering instead) ?

@mixxxbot
Copy link
Collaborator Author

Commented by: nm2107
Date: 2022-06-30T20:20:43Z


Do we need this condition https://github.com/mixxxdj/mixxx/blob/2.3.3/src/engine/enginebuffer.cpp#L1260-L1270 if the one right above checks for deck passthrough already ?

@mixxxbot
Copy link
Collaborator Author

Commented by: nm2107
Date: 2022-07-01T07:54:39Z


Meanwhile I made myself a patch commenting the condition : nm2107/org.mixxx.Mixxx@b5f356d

This temporarily fixes my issue, but I need some advice for a proper fix.

@mixxxbot
Copy link
Collaborator Author

Commented by: uklotzde
Date: 2022-07-01T23:11:42Z


The engine code is old with lots of undocumented dependencies and unknown side effects that no one is aware of. Fixing one bug might cause new bugs.

I suppose no one is able to give advice on how to fix this properly. It is pure trial and error while trying to understand the code.

@mixxxbot mixxxbot transferred this issue from another repository Aug 24, 2022
@JoergAtGithub
Copy link
Member

@nm2107 Could you check if this Mixxx 2.3.3 regression still occurs with Mixxx 2.4 beta builds?

@daschuer daschuer added this to the 2.4.0 milestone Aug 13, 2023
@nm2107
Copy link

nm2107 commented Aug 16, 2023

Hello :)
I don't have my hands on the decks right now, I'll try it when possible. Thank you for the suggestion.

@daschuer
Copy link
Member

daschuer commented Sep 3, 2023

@nm2107 did you have any test results?

@nm2107
Copy link

nm2107 commented Sep 4, 2023

Hello :)

I've checked out the 2.4-beta tag (commit 592c0c7 ) and built the project. Unfortunately the issue is still here (same behavior detected as in the original post).

@Swiftb0y
Copy link
Member

Swiftb0y commented Sep 4, 2023

hey there @nm2107 I'm trying to reproduce the issue but I can't seem to do so. I have a couple questions: What vinyl do you use? What are your setting (see below example from my screenshot from latenight)? In step 3 you pressing the cue button, I assume the one above the playbutton, not in the vinyl settings, right? In that step you are also describing that the playback stops (which is not the case for me as the vinyl obviously keeps spinning). What is your "Cue Mode" (as in Settings > Decks > Cue Mode)? It would be easier to reproduce if I could match my setup closer to yours.
Screenshot from 2023-09-04 22-42-22

@nm2107
Copy link

nm2107 commented Sep 5, 2023

Hello :)

The issue disappears when commenting this code block.

To answer your questions, here is my vinyl control settings panel :
Screenshot from 2023-09-05 19-56-10

And the CUE mode setting :

Screenshot from 2023-09-05 19-59-14

I am using a pair of Denon SC3900 decks for DVS (similar to DVS using CDs, expect that the decks have a built-in feature to send audio signal, i.e. no need of any CD). (see my repo for the midi mappings).

So when I press the CUE button on the decks, it acts like when I press the CUE button on a classic CD deck : stops the playback of the audio signal, and I configured my MIDI mapping to go back to the CUE point (c.f. the repo linked above).

In step 3 you pressing the cue button, I assume the one above the playbutton

Yes, the physical CUE button on the deck, above the play button.

I hope it helps !

EDIT : I don't use the passthrough mode, I enable vinyl control for decks 1 and 2 (by using ctrl+t, ctrl+y shortcuts)

@JoergAtGithub
Copy link
Member

Independed of your issue, if the donuts appear in yellow, something is wrong with your setup. They must appear green. Does the color change, if you choose another "Vinyl Type"?

@Swiftb0y
Copy link
Member

Swiftb0y commented Sep 6, 2023

To answer your questions, here is my vinyl control settings panel

Thank you, but that was specifically not what I was asking about. I was asking about the "vinyl control options" which are configured in the skin. As the placement of these options depends on the skin, what skin are you using may I ask?

@Swiftb0y
Copy link
Member

Swiftb0y commented Sep 6, 2023

The issue disappears when commenting this code block.

That is good to know, but in order to properly fix this, I will first need to reproduce the issue myself, otherwise I can't confirm the source of the issue nor whether the fix is adequate.

@nm2107
Copy link

nm2107 commented Sep 6, 2023

Yes, sorry about the lack of precision, here are the details :

Screenshot from 2023-09-06 19-04-40

Screenshot from 2023-09-06 19-06-39
(for both decks when vinyl control is enabled on both decks)

if the donuts appear in yellow, something is wrong with your setup. They must appear green. Does the color change, if you choose another "Vinyl Type"?

Still yellow for any other vinyl type. The Serato CD option matches the config I've set on the physical decks (c.f this manual). The decks emit an audio signal at a given frequency, which should match the one used by the Serato CD. Maybe the yellow color is here to indicate a lack of timecode (i.e. only relative freq info but no absolute timecode to indicate about track position). But still, this is not related to the issue posted here, as you noticed ;) Thank you tho for pointing this out :)

@nm2107
Copy link

nm2107 commented Sep 6, 2023

That is good to know

Does mixxx have some kind of logger where I could print the values used in the condition that I commented ? Perhaps it would help to know why the condition is entered.

@Swiftb0y
Copy link
Member

Swiftb0y commented Sep 6, 2023

Does mixxx have some kind of logger where I could print the values used in the condition that I commented ? Perhaps it would help to know why the condition is entered.

If you're asking on how to do printf-debugging in mixxx, we use Qt QDebug facilities. eg qDebug() << some_variable << some_other_variable;

@JoergAtGithub
Copy link
Member

Maybe the yellow color is here to indicate a lack of timecode (i.e. only relative freq info but no absolute timecode to indicate about track position).

Yes, yellow means, that there is one of the components missing. Since we see the donut, it must be the timecode with the absolute position information.

@nm2107
Copy link

nm2107 commented Sep 6, 2023

Thanks ;)

By reading the original comments of this issue, it appears that the condition was introduced to deal with an OOM issue with the waveform renderer in some cases (i.e. prevent the redraw call -> no more OOM). In that case, why not to try to fix the OOM and to get rid of this condition ?

An other idea would be to call the waveform redraw on MIDI message input (e.g. when a hotcue is pressed), or on any other user input (e.g. when clicking on a hotcue on the mixxx UI).

Note that only commenting this line fixes the issue.

@nm2107
Copy link

nm2107 commented Sep 8, 2023

I don't know if some other users are impacted by this regression, but feel free to skip this bug for the 2.4.0 milestone if you can't reproduce it. I'll update my fork with the commented line for my personal use case when 2.4.0 will be released ;)

@daschuer
Copy link
Member

The Issue has been introduced here:
8757d43
9360294
To fix a bug, that has been later fully fixed here:
d051deb

@Swiftb0y
Copy link
Member

fixed by #11977. Feel free to reopen if not.

@nm2107
Copy link

nm2107 commented Sep 13, 2023

Thank you guys for merging the fix :D !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants