[mythtv] OSX backend beta test

[David Abrahams]

Isaac Richards <ijr at case.edu> writes:

> On Sunday 08 January 2006 19:05, David Abrahams wrote:
>> Isaac Richards <ijr at case.edu> writes:
>> > On Saturday 07 January 2006 21:56, David Abrahams wrote:
>> >> Okay, I got it working, as far as I can tell; the patch against this
>> >> morning's source (Version 8516) is enclosed.
>> >
>> > Comments:
>> >
>> > - The amount of ifdefs you've added are not good.  Create new classes
>> > when the code is significantly different, as it is in this case.
>>
>> Sure, I'll be happy to refactor it; I just wanted to get something
>> working first, and I expected more code to be shared than actually is.
>
> Would've expected it would have been easier separated out to begin with, but 
> however you want to do things is fine.

As I do my refactoring, it begins to look to me as though the original
FirewireChannel code has a few problems.  Close() doesn't seem to do
anything, for example.  Also, if the channel failed to open,
SetChannelByString generates an error message about having the wrong
kind of device and then returns true.

There are no documented requirements for SetChannelByString, so I
could be wrong about what it's supposed to do, of course.  Maybe
returning true in that case is just fine.  And maybe it doesn't matter
if Close() doesn't release the firewire handle allocated in the
channel's Open() call.  Can you clarify?

>> > - I assume that this should be compile-time optional, and it's not?
>>
>> I don't know what you mean, exactly.  It might be very obvious to
>> everyone else what you mean by "this" and "compile-time optional," but
>> being new to MythTV culture I need a little more explanation.  Are you
>> saying that
>>    configure --disable-firewire
>>
>> should cause all of this new code not to be included?  I admit I
>> haven't yet considered whether configure is as fine-grained as
>> possible.
>
> Yes.  Additional unnecessary dependencies should be always be optional.

Okay, I'll take care of it, thanks.

>> > - I'd prefer not to have more device-specific code in tv_rec than there
>> > already is.
>>
>> I suppose you're referring to the inclusion of the
>> AVCDeviceController* in that class?
>
> Yes.
>
>> If so, I understand; that class 
>> is already looking pretty scary.  I actually started without it in
>> there, to avoid intruding.  Here's the problem: the Channel changer
>> and the Recorder must share the same device controller.  Otherwise,
>> the OS tells you that you're trying to open a device twice with
>> exclusive access and nothing gets recorded.  Where would you like to
>> see the shared data stored?  I could keep it in the Channel, but then
>> I'd need to expose some way to get the channel from the TVRec, which I
>> wasn't sure would be well received either -- and it would require a
>> downcast to get it from the ChannelBase to FirewireChannel, which ends
>> up making some assumptions about the operation of TVRec (e.g. that the
>> Channel is initialized first), which I didn't like much either.
>
> Recorder specific data structures belong in the recorder specific code - the 
> other recorders already share data between the recorder and channel classes.  
> Shouldn't be difficult to copy that.

I see what's happening there, but I'd like some rationale.  It doesn't
seem to be a big win to add public GetXXXChannel members to the public
interface of TVRec.  Why not let the Recorder instances do the
downcast themselves, to get the sort of channel they're looking for?

Also, is the Channel always allocated first?  Is the Recorder supposed
to work if the Channel couldn't be allocated or opened?

Looking more closely, it seems like TVRec could benefit greatly by
factoring out all the functionality that depends on genopt.cardtype
into a family of related classes for each card type but that's a
project for another day.

-- 
Dave Abrahams
Boost Consulting
www.boost-consulting.com