Opened 18 years ago
Closed 18 years ago
Last modified 18 years ago
#850 closed enhancement (fixed)
Inaccuracies in audio timecode calculations
Reported by: | 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)
Change History (14)
Changed 18 years ago by
Attachment: | audiotimecode.patch added |
---|
comment:1 Changed 18 years ago by
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 18 years ago by
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 18 years ago by
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 18 years ago by
Attachment: | audiotimecode_v2.patch added |
---|
Updated patch to fix transcode compilation error
comment:4 Changed 18 years ago by
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 18 years ago by
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 18 years ago by
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 18 years ago by
Milestone: | → 0.20 |
---|---|
Type: | defect → enhancement |
comment:8 Changed 18 years ago by
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 18 years ago by
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 18 years ago by
Owner: | changed from Isaac Richards to danielk |
---|
comment:11 Changed 18 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(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.
audio timecode patch