Opened 3 years ago

Closed 4 months ago

#12973 closed Patch - Bug Fix (fixed)

Repeated use of time stretch adjustment has rounding error

Reported by: amb@… Owned by: Klaas de Waal
Priority: minor Milestone: 31.0
Component: MythTV - General Version: 0.28.0
Severity: low Keywords:
Cc: Ticket locked: no

Description

When repeatedly adjusting the time stretch (faster and then slower) it can introduce a rounding error due to the use of floating point without re-quantisation. For example the on-screen display of the time stretch can show weird numbers like "1.0499999995" instead of "1.05".

Attachments (1)

time-stretch-rounding.diff (514 bytes) - added by amb@… 3 years ago.

Download all attachments as: .zip

Change History (9)

Changed 3 years ago by amb@…

Attachment: time-stretch-rounding.diff added

comment:1 Changed 3 years ago by Stuart Auchterlonie

Milestone: unknown0.28.1
Owner: set to Stuart Auchterlonie
Status: newaccepted

comment:2 Changed 3 years ago by Stuart Auchterlonie

Milestone: 0.28.129.0

comment:3 Changed 2 years ago by Stuart Auchterlonie

Milestone: 29.029.1

comment:4 Changed 2 years ago by Stuart Auchterlonie

Milestone: 29.10.28.2

Moving remaining open tickets to 0.28.2 milestone

comment:5 Changed 2 years ago by Stuart Auchterlonie

Milestone: 0.28.229.2

Moving remaining open tickets to 29.2 milestone

comment:6 Changed 5 months ago by Stuart Auchterlonie

Milestone: 29.231.0
Owner: changed from Stuart Auchterlonie to Klaas de Waal
Status: acceptedassigned

comment:7 Changed 5 months ago by Klaas de Waal

The problem has been reproduced on today's master. The fix as given, with minor modifications to avoid compiler warnings, does remove the floating point error accumulation inherent in repeated additions and subtractions. The fix as tested is here:

    ctx->m_tsNormal = 0.05*(int)(new_ts_normal * 20.0F + 0.5F);

However, this does still not guarantee that the displayed value is limited to two decimal places. This can be specified in the QString::number conversion that is used to generate the string for presentation, as applied in the actual code here:

            UpdateOSDStatus(ctx, tr("Adjust Time Stretch"), tr("Time Stretch"),
                            QString::number(ctx->m_tsNormal,'f',2),
                            kOSDFunctionalType_TimeStretchAdjust, "X",
                            (int)(ctx->m_tsNormal*(1000/kTimeStretchMax)),
                            kOSDTimeout_None);

An additional benefit of having always two decimal places is that the "Time Stretch..." portion of the message "Adjust Time Stretch....Time Stretch 1.10X" presentation has a stable position on the screen because of the fixed length of the string.

comment:8 Changed 4 months ago by Klaas de Waal <kdewaal@…>

Resolution: fixed
Status: assignedclosed

In dae831860b/mythtv:

On-screen display of time stretch in playback

Reduce floating point inaccuracy in the time stretch value
caused by repeatedly changing the value.
This is the patch proposed by amb@… but with
a numeric constant for the time stretch step value so that
it is defined in only one place.
Change the presentation of the time stretch value
to show always two decimal places.

Fixes #12973

Note: See TracTickets for help on using tickets.