Opened 13 years ago

Closed 13 years ago

Last modified 13 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@… 13 years ago.
patch to audiooutputbase.h to change visibility of member variables
audiooutputca.h.diff (756 bytes) - added by awk@… 13 years ago.
patch to audiooutputca.h to override Set/GetAudiotime? and member timestamp
audiooutputca.cpp.diff (2.9 KB) - added by awk@… 13 years ago.
patch to audiooutputca.cpp to override Set/GetAudiotime?
audiooutputbase.h.v2.diff (630 bytes) - added by awk@… 13 years ago.
revised patch to audiooutputbase.h
audiooutputca.cpp.v2.diff (2.9 KB) - added by awk@… 13 years ago.
revised patch to audiooutputca.cpp

Download all attachments as: .zip

Change History (12)

Changed 13 years ago by awk@…

Attachment: audiooutputbase.h.diff added

patch to audiooutputbase.h to change visibility of member variables

Changed 13 years ago by awk@…

Attachment: audiooutputca.h.diff added

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

Changed 13 years ago by awk@…

Attachment: audiooutputca.cpp.diff added

patch to audiooutputca.cpp to override Set/GetAudiotime?

comment:1 Changed 13 years ago by Nigel

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 Changed 13 years ago by Nigel

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

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

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

Attachment: audiooutputbase.h.v2.diff added

revised patch to audiooutputbase.h

Changed 13 years ago by awk@…

Attachment: audiooutputca.cpp.v2.diff added

revised patch to audiooutputca.cpp

comment:5 Changed 13 years ago by Nigel

Resolution: fixed
Status: assignedclosed

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

comment:6 Changed 13 years ago by Nigel

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

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

Yep looks fine.

Note: See TracTickets for help on using tickets.