Opened 19 years ago

Closed 19 years ago

Last modified 19 years ago

#2265 closed patch (fixed)

Improved audio timestamp calculation on Mac OS X

Reported by: awk@… 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)

audiooutputbase.h.diff (1.6 KB ) - added by awk@… 19 years ago.
patch to audiooutputbase.h to change visibility of member variables
audiooutputca.h.diff (756 bytes ) - added by awk@… 19 years ago.
patch to audiooutputca.h to override Set/GetAudiotime and member timestamp
audiooutputca.cpp.diff (2.9 KB ) - added by awk@… 19 years ago.
patch to audiooutputca.cpp to override Set/GetAudiotime
audiooutputbase.h.v2.diff (630 bytes ) - added by awk@… 19 years ago.
revised patch to audiooutputbase.h
audiooutputca.cpp.v2.diff (2.9 KB ) - added by awk@… 19 years ago.
revised patch to audiooutputca.cpp

Download all attachments as: .zip

Change History (12)

by awk@…, 19 years ago

Attachment: audiooutputbase.h.diff added

patch to audiooutputbase.h to change visibility of member variables

by awk@…, 19 years ago

Attachment: audiooutputca.h.diff added

patch to audiooutputca.h to override Set/GetAudiotime and member timestamp

by awk@…, 19 years ago

Attachment: audiooutputca.cpp.diff added

patch to audiooutputca.cpp to override Set/GetAudiotime

comment:1 by Nigel, 19 years ago

Owner: changed from Isaac Richards to Nigel
Status: newassigned

This may be a bit risky for 0.20, but I will certainly start applying/testing locally.

comment:2 by Nigel, 19 years ago

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 by awk@…, 19 years ago

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 by awk@…, 19 years ago

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.

by awk@…, 19 years ago

Attachment: audiooutputbase.h.v2.diff added

revised patch to audiooutputbase.h

by awk@…, 19 years ago

Attachment: audiooutputca.cpp.v2.diff added

revised patch to audiooutputca.cpp

comment:5 by Nigel, 19 years ago

Resolution: fixed
Status: assignedclosed

(In [11165]) Improved audio timestamp calculation on Mac OS X. Closes #2265

comment:6 by Nigel, 19 years ago

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 by awk@…, 19 years ago

Cc: nigel@… removed
Keywords: OSX GO7007 Capture removed

Yep looks fine.

Note: See TracTickets for help on using tickets.