Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#850 closed enhancement (fixed)

Inaccuracies in audio timecode calculations

Reported by: mythtv@… Owned by: danielk
Priority: minor Milestone: 0.20
Component: mythtv Version: head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

The existing audio code accumulates the timecode (in milliseconds) , this approach is insufficiently accurate as the rounding errors also accumulate over time. This most obviously manifests itself as the elapsed time in MythMusic for a 44.1kHz track doesn't match the overall track length at the end of the track playback. But would almost certainly cause all sorts of havoc for video streams with audio sample rates that don't divide nicely into an integer number of milliseconds for the block sizes used.

The attached patch stores the position in bytes (as opposed to milliseconds) thus eliminating the rounding errors.

<This is my first patch, so I hope that it's the correct format etc. ;-)>

Attachments (2)

audiotimecode.patch (7.5 KB) - added by mythtv@… 14 years ago.
audio timecode patch
audiotimecode_v2.patch (8.0 KB) - added by mythtv@… 14 years ago.
Updated patch to fix transcode compilation error

Download all attachments as: .zip

Change History (14)

Changed 14 years ago by mythtv@…

Attachment: audiotimecode.patch added

audio timecode patch

comment:1 Changed 14 years ago by paulh

Fails to compile with this error:

transcode.cpp:141: error: conflicting return type specified for `virtual int
   AudioReencodeBuffer::GetAudiotime()'
../../libs/libmyth/audiooutput.h:54: error:   overriding `virtual long long int
   AudioOutput::GetAudiotime()'
make[2]: *** [transcode.o] Error 1
make[2]: Leaving directory `/media/src/mythtv-build/mythtv/programs/mythtranscode'
make[1]: *** [sub-mythtranscode] Error 2
make[1]: Leaving directory `/media/src/mythtv-build/mythtv/programs'
make: *** [sub-programs] Error 2

comment:2 Changed 14 years ago by Isaac Richards

I'm pretty sure the reordering of logic is incorrect, as well. Might well work for mythmusic, but I doubt it will for tv.

comment:3 Changed 14 years ago by paulh

Changing line 140 of transcode.cpp to

    virtual long long GetAudiotime(void)

allows it to compile.

I can confirm it fixes the timing in mythmusic. Before this patch the time could be something like 12 seconds out for a 4 minute track. Now the time is spot on.

Isaac don't worry I'm not going to commit this I will leave that to you or Daniel to review. I have tried both live TV and playing back recordings both from a PVR250 and from DVB-t and everything seems to be working fine. Even using time stretch to play a recording worked fine. Not sure about mythtranscode.

Changed 14 years ago by mythtv@…

Attachment: audiotimecode_v2.patch added

Updated patch to fix transcode compilation error

comment:4 Changed 14 years ago by mythtv@…

Issac,

I would be the first to admit that the patch isn't particularly elegant. I will continue to investigate this area of code to see if I can obtain a better understanding to its intended functionality. But the patch appears to work correctly for me, both in MythMusic and TV playback.

I think that the system could be clarified and documented, which I intend to have a go at, but at the moment I am simply trying to understand the existing operation.

comment:5 Changed 14 years ago by Mark Spieth

Ive done some testing with both algo's side by side and looking at the timecodes generated at SetAudiotime? and there is no difference for the test video I was using. using audio as timebase. thus I would conclude this is hardware/driver related. The long long change for GetAudioTime? is good though but wont exhibit the indicated fault.

comment:6 Changed 14 years ago by mythtv@…

Version: head

Mark,

Thank you for taking the time to investigate my patch.

I am happy that your testing revealed no noticeable change in the behaviour of the 2 algorithms, I hope that this indicates that my patch is working correctly; video playback was working correctly for me before I developed the patch. The original bug I set out to resolve was the indicated playback position for tracks played from within MythMusic.

Through my investigation I discovered the root cause of that fault to lie within the "AudioOutputBase?" class and the way that it keeps track of the current playback position ("AudioTime?"), which is basically achieved by accumulating the time in milliseconds represented by each buffer of audio submitted to the class via one of the "AddSamples?" member functions. The audio timecode represented by the last sample stored in the audio cyclic buffer ("audiobuffer") is accumulated into the "audbuf_timecode" member variable by lines 626 through 633 of the original code:-

    if (timecode < 0) 
        timecode = audbuf_timecode; // add to current timecode
    
    /* we want the time at the end -- but the file format stores
       time at the start of the chunk. */
    // even with timestretch, timecode is still calculated from original
    // sample count
    audbuf_timecode = timecode + (int)((samples * 100000.0) / effdsp);

For video playback (I am based in the UK with a DVB-T setup):-

Audio is 16bit, stereo, at 48000Hz

timecode = is always provided (i.e. is positive)
samples = (in my testing at least) was always 1152
effdsp = 100 * audio_samplerate

audbuf_timecode = timecode + (int)((1152 * 100000.0)/(100 * 48000)
audbuf_timecode = timecode + (int)(24)

As such everything works as expected.........

Now for audio playback in MythMusic (playing FLAC encoded CD tracks):-

Audio is 16bit, stereo, 44100Hz

timecode = is not provided (i.e. is "-1")
samples = (in my testing at least) was always 512
effdsp = 100 * audio_samplerate

audbuf_timecode = audbuf_timecode + (int)((512 * 100000.0)/(100 * 44100)
audbuf_timecode = audbuf_timecode + (int)(11.60997732426303855)

As such, each audio buffer provided to the AudioOutputBase? class is actually approximately 0.6 milliseconds longer than it is given credit for resulting in the misreporting of playback position within MythMusic (which is based upon the figures provided by the AudioOutputBase? class).

The "int" to "long long" return type from the "GetAudioTime?" function was a change that I noticed along the way, and thought I would fix it as it might have strange ramifications that might otherwise be hard to replicate and fix.

My patch is an initial attempt to rectify the behaviour of the AudioOutputBase? class for the situations within which it is currently being used. I would like to take an opportunity to reconsider its current interface, possibly make some changes, and document the whole thing. But as I have said, this patch was my first attempt at contributing to the MythTV project and I didn't want to propose any major changes until I understand more about the context within which this code operates. I have some experience of writing audio processing code, I write soundcard drivers for a living, but I am less familiar with the video decoding and corresponding syncronisation issues.

It is probably possible to rectify the MythMusic behaviour without touching the existing mechanism for reporting timecode for the video playback case. I would not be surprised if the video playback case always arranges that the audio buffer supplied to the "AddSamples?" function consists of an integer number of milliseconds worth of data. But as this was beyond my knowledge and I always consider it to be good to avoid any such assumptions where at all possible I proposed the alterations I have.

Best regards

Peter

comment:7 Changed 14 years ago by danielk

Milestone: 0.20
Type: defectenhancement

comment:8 Changed 14 years ago by Mark Spieth

I have an easy way to fix this audbuf_timecode should be stored with resolution *1000 (i.e. 1000 better than now) minimal changes and then quite accurate without byte counting. will get to it after AC3 pt renec.

comment:9 Changed 14 years ago by mythtv@…

When so much of the code within that class is dedicated to ensuring that the timecode produced is very accurate (with going to the lengths of using a high resolution timer to estimate the soundcard’s position based upon elapsed time since the position was previously reported). I don’t under stand why the solution that was merely ‘more’ accurate then the existing code, especially when an implementation that is completely accurate has been provided.

If you can express some details regarding your apparent objections to my proposed modifications then I would be happy to attempt to address your concerns.

I’d also dispute that it’s an “enhancement”; it’s a bug, it’s not a serious bug (at the moment), but it definitely a bug.

comment:10 Changed 14 years ago by danielk

Owner: changed from Isaac Richards to danielk

comment:11 Changed 14 years ago by danielk

Resolution: fixed
Status: newclosed

(In [9770]) Closes #850. Fixes rounding error in timecode calculation for streams without an explicit timecode.

This applies the patch from mythtv At dadeos DT freeserve dot co dt uk, but without the changes to the signature of getAudiotime(). The current return type of int can handle 500 hours of audio without wackyness, and if we are going to change the binary revision we should fix the constness and return types of the rest of the methods in the audio classes.

comment:12 Changed 14 years ago by Isaac Richards

(In [9772]) Much less intrusive version of r9770. Still fixes #850.

Note: See TracTickets for help on using tickets.