Opened 18 years ago
Closed 18 years ago
Last modified 18 years ago
#2265 closed patch (fixed)
Improved audio timestamp calculation on Mac OS X
Reported by: | Owned by: | Nigel | |
---|---|---|---|
Priority: | minor | Milestone: | unknown |
Component: | mythtv | Version: | |
Severity: | medium | Keywords: | |
Cc: | Ticket locked: | no |
Description
audiooutputca.cpp uses a combination of two timing methods, AudioGetCurrentHostTime?() and gettimeofday() to calculate the timestamp of the currently playing audio. This can lead to errors in AV sync since the two calls cannot reliably be combined (gettimeofday is also potentially less efficient). Lastly the current base class implementation of SetAudiotime? acquires a pair of locks which may cause priority inversion on Mac OS X when called from the high priority audio rendering thread provided by the OS.
The attached patch to audiooutputca overrides SetAudiotime? and GetAudiotime? to use AudioGetCurrentHostTime? without acquiring a lock (the single 32bit variable used doesn't need the protection). This does also however require changing the visibility of pSoundStretch, audbuf_timecode and audiotime from private to protected so that the derived class can access them - hence the patch to audiooutputbase.h
Attachments (5)
Change History (12)
Changed 18 years ago by
Attachment: | audiooutputbase.h.diff added |
---|
Changed 18 years ago by
Attachment: | audiooutputca.h.diff added |
---|
patch to audiooutputca.h to override Set/GetAudiotime? and member timestamp
Changed 18 years ago by
Attachment: | audiooutputca.cpp.diff added |
---|
patch to audiooutputca.cpp to override Set/GetAudiotime?
comment:1 Changed 18 years ago by
Owner: | changed from Isaac Richards to Nigel |
---|---|
Status: | new → assigned |
This may be a bit risky for 0.20, but I will certainly start applying/testing locally.
comment:2 Changed 18 years ago by
1) Making AudioOutputCA a friend of AudioOutputBase? allows the new implementations to access those three variables, and seems a neater than changing their scope/accessibility?
2) In my testing, the behaviour seems identical, but then I am not trying to grab stuff in a backend too!
3) Andrew, I am ready to commit, but if you can wait until after the feature freeze for 0.20, that would be better.
comment:3 Changed 18 years ago by
As an alternative to a review of the class hierarchy etc. I guess the friend approach is OK. Although I have to say that it feels 'odd' to me to make a subclass a friend rather than either change the class design (or provide accessors to that data if it's truly supposed to be private). In fact that's probably the best approach - add a couple of protected accessors in the base class that derived classes can use to get at the variables - if changing the visibility is accessible.
Regarding behaviour - are you using a backend which inherently delivers MPEG Transport Streams ? Something like the Firewire recorder interface to a settop box ? It may be in such a circumstance the audio sync is less of an issue.
I'm fine with waiting 'til post 0.20 I doubt that I'll have finished prepping the pretty large amount of backend changes I have in time - let alone anyone reviewing them and wanting to commit them in that timeframe.
comment:4 Changed 18 years ago by
Cc: | nigel@… added |
---|---|
Keywords: | OSX GO7007 Capture added |
I've revised the patch a little bit to add 'base class' accessors to the audiooutbase.h header and to call them from the derived class. That way I don't need to directly change the visibility of the variables, and there's no need for a friend either.
Note the accessor functions are still protected - they're only intended for derived class use.
Changed 18 years ago by
Attachment: | audiooutputbase.h.v2.diff added |
---|
revised patch to audiooutputbase.h
Changed 18 years ago by
Attachment: | audiooutputca.cpp.v2.diff added |
---|
revised patch to audiooutputca.cpp
comment:5 Changed 18 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:6 Changed 18 years ago by
I removed the comments that were copied from the base class, and added a few Doxygen comments. Apart from that, it should be about the same as your version, Andrew?
comment:7 Changed 18 years ago by
Cc: | nigel@… removed |
---|---|
Keywords: | OSX GO7007 Capture removed |
Yep looks fine.
patch to audiooutputbase.h to change visibility of member variables