Enhance precision of track duration #970

uklotzde · 2016-06-25T08:21:55Z

Launchpad
https://bugs.launchpad.net/mixxx/+bug/1497183

Track Property
Changed the data type of this property. In JavaScript it's a number and and control objects are always double.

DB Column
Since the existing DB column 'duration' cannot be deleted a new column 'duration_real' that gets it's default value from 'duration' is added. Both columns are written, but only the new column is read.

Util Classes
I've moved the formatting of duration values into util/duration.h. The header util/time.h caused naming conflicts with system headers when included in track/track.h. I don't wanted to fix this now. We should think about enclosing more classes in namespace 'mixxx'. I've used 'Mixxx' in the past, but lower case names are more common for C++. The namespace fixes will be part of a following PR

Requires TagLib v1.10 or newer.

Ubuntu 14.04 only provides TagLib 1.9.x

daschuer · 2016-06-25T16:03:12Z

src/library/autodj/autodjprocessor.cpp

-            m_nextTransitionTime = math_min(m_iTransitionTime,
-                                            fromTrackDuration / 2);
+            m_nextTransitionTime = math_min(m_transitionTime,
+                                            fromTrackDuration / 2.0);


A Pentium is able to divide a float by an integer. No need for .0 here.

daschuer · 2016-06-25T16:18:29Z

src/track/trackmetadatataglib.cpp

    pTrackMetadata->setBitrate(audioProperties.bitrate());
+#if TAGLIB_HAS_LENGTH_IN_MILLISECONDS
+    double duration = audioProperties.lengthInMilliseconds() / 1000.0;


ähh * 1000.0?

daschuer · 2016-06-25T17:33:54Z

Thank you for adopting this.

Even though a sqlite column is marked at int, you are able to store a real value.
See http://sqlite.org/datatype3.html#section_3
The important idea here is that the type is recommended, not required. Any column can still store any type of data.

I have just checked. You are able to store 90.5 in the old duration column. It is read as 91 in current Mixxx. If you store 90.49 it is read as 90.

The question is, can't we just ignore the "interger recommendation" and continue the original duration column?
Of cause this is less clean because we do not follow the legacy type recommendation, but it avoids to carry a useless column around which may have a performance or size impact.
Unfortunately sqlite does not allow to remove the recommendation.

What do you think?

rryan · 2016-06-25T18:30:24Z

I think it's safe to ignore the integer recommendation -- SQLite wouldn't be able to change that without breaking backwards compatibility so we don't have to worry that they will in the future I think. It would be nice to not have the deprecated column.

For newly created DB files use the correct type for the schema. Existing DB files are backwards compatible according to the documentation: https://www.sqlite.org/datatype3.html "Any column in an SQLite version 3 database, except an INTEGER PRIMARY KEY column, may be used to store a value of any storage class."

uklotzde · 2016-06-25T19:35:37Z

Thanks for the hint, Daniel! Let's reuse the existing column, it's even
officially documented:

https://www.sqlite.org/datatype3.html
"Any column in an SQLite version 3 database, except an INTEGER PRIMARY KEY
column, may be used to store a value of any storage class."

On 25.06.2016 19:33, Daniel Schürmann wrote:

Thank you for adopting this.

Even though a sqlite column is marked at int, you are able to store a real
value.
See http://sqlite.org/datatype3.html#section_3
|The important idea here is that the type is recommended, not required.
Any column can still store any type of data.|

I have just checked. You are able to store 90.5 in the old duration
column. It is read as 91 in current Mixxx. If you store 90.49 it is read
as 90.

The question is, can't we just ignore the "interger recommendation" and
continue the original duration column?
Of cause this is less clean because we do not follow the legacy type
recommendation, but it avoids to carry a useless column around which may
have a performance or size impact.
Unfortunately sqlite does not allow to remove the recommendation.

What do you think?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#970 (comment), or
mute the thread
https://github.com/notifications/unsubscribe/ABZ-at2A0Kt23vWF0_-D6-iA980kXi-4ks5qPWaGgaJpZM4I-TBY.

daschuer · 2016-06-25T23:36:37Z

src/library/dao/trackdao.cpp

@@ -1279,7 +1279,7 @@ TrackPointer TrackDAO::getTrackFromDB(TrackId trackId) const {
        { "rating", setTrackRating },
        { "comment", setTrackComment },
        { "url", setTrackUrl },
-        { "duration", setTrackDuration },
+        { "duration_real", setTrackDuration },


daschuer · 2016-06-25T23:42:54Z

That was fast :-) Thank you very much! LGTM

I believe this work-around is no longer necessary after this bug was fixed: https://bugs.launchpad.net/mixxx/+bug/1497183 in mixxxdj#970

uklotzde added 5 commits June 25, 2016 10:12

Move formatting of durations from Time into Duration class

4e82485

Calculate duration (in seconds) as double value in AudioSource

b0debc7

Enhance precision of track duration from int to double

8ce8f1f

Read track duration with millisecond precision from TagLib

1d8d933

Requires TagLib v1.10 or newer.

Fix build for older versions of TagLib before 1.10

db2e002

Ubuntu 14.04 only provides TagLib 1.9.x

daschuer reviewed Jun 25, 2016
View reviewed changes

uklotzde added 2 commits June 25, 2016 18:11

Use integer divisor

99f990a

Remove obsolete cast to double

c160ca9

daschuer reviewed Jun 25, 2016
View reviewed changes

Replace magic number with constant

86c9a63

daschuer reviewed Jun 25, 2016
View reviewed changes

Fix a leftover from the previous revert commit

1070a48

daschuer merged commit 89612ff into mixxxdj:master Jun 25, 2016

uklotzde deleted the duration_real branch June 26, 2016 06:20

radusuciu added a commit to radusuciu/mixxx that referenced this pull request Feb 14, 2017

Get track duration directly from engine

3afdf17

I believe this work-around is no longer necessary after this bug was fixed: https://bugs.launchpad.net/mixxx/+bug/1497183 in mixxxdj#970

radusuciu added a commit to radusuciu/mixxx that referenced this pull request Feb 15, 2017

Get track duration directly from engine

ccc8105

I believe this work-around is no longer necessary after this bug was fixed: https://bugs.launchpad.net/mixxx/+bug/1497183 in mixxxdj#970

radusuciu added a commit to radusuciu/mixxx that referenced this pull request Feb 16, 2017

Get track duration directly from engine

9382d64

I believe this work-around is no longer necessary after this bug was fixed: https://bugs.launchpad.net/mixxx/+bug/1497183 in mixxxdj#970

ninomp mentioned this pull request Nov 5, 2017

Fix rename detect feature #1373

Merged

mixxxbot mentioned this pull request Aug 22, 2022

"duration" is an integer, should be double #8229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance precision of track duration #970

Enhance precision of track duration #970

uklotzde commented Jun 25, 2016 •

edited

daschuer Jun 25, 2016

daschuer Jun 25, 2016

daschuer Jun 25, 2016

daschuer commented Jun 25, 2016

rryan commented Jun 25, 2016

uklotzde commented Jun 25, 2016

daschuer Jun 25, 2016

daschuer commented Jun 25, 2016

Enhance precision of track duration #970

Enhance precision of track duration #970

Conversation

uklotzde commented Jun 25, 2016 • edited

daschuer Jun 25, 2016

Choose a reason for hiding this comment

daschuer Jun 25, 2016

Choose a reason for hiding this comment

daschuer Jun 25, 2016

Choose a reason for hiding this comment

daschuer commented Jun 25, 2016

rryan commented Jun 25, 2016

uklotzde commented Jun 25, 2016

daschuer Jun 25, 2016

Choose a reason for hiding this comment

daschuer commented Jun 25, 2016

uklotzde commented Jun 25, 2016 •

edited