Feedback: New DVD Interface

[Shonumi]

So I managed to put down Xenoblade long enough to work on the DVD interface. So far so good, at least .elf files load, gonna get on .gcm/.iso files next.

I thought the way input_common handled its interface was really nice. We just derived whatever type of input (keyboard or joystick) we needed from a common class and set a pointer to that instance. That way of doing things made a lot of sense to me, and I think it's well suited to the DVD interface. Function pointers are cool, but virtual class functions would be more consistent with other parts of the code.

Here's the DVDBase class from the WIP realdvd.h I've whipped up (no pun intended ) :

Code:

class DVDBase {
public:
    DVDBase();
    virtual ~DVDBase();

    virtual u32 Read(u32 FilePtr, u32 *MemPtr, u32 Len);
    virtual u32 Seek(u32 FilePtr, u32 SeekPos, u32 SeekType);
    virtual u32 Open(char *filename);
    virtual u32 GetFileSize(u32 FilePtr);
    virtual u32 Close(u32 FilePtr);
    virtual u32 GetPos(u32 FilePtr);
    virtual u32 ChangeDir(char *ChangeDirPath);
    virtual void Reset();
    virtual int Load(char *filename);

private:
    DISALLOW_COPY_AND_ASSIGN(DVDBase);
};

So, as it stands, DVDBase doesn't do too much new, aside from replacing function pointers altogether, and giving us a load function that's independent of what's being loaded (e.g. .elf, .gcm, doesn't matter). I also made a DVDBase pointer, called g_real_dvd (part of the dvd namespace, of course) that can be set to instances of derived classes (DVDELF, DVDGCM, etc). Oh yeah, function names are a bit more concise too.

Right now, it functions exactly the same as the existing code, and that's where my question is. Do you guys want this new interface to do anything different from the old one? Or do you just want the new interface to be cleaned up and such, while leaving functionality as is? I just started so I'm not afraid to change some things (or everything ).

[ShizZy]

Good start. I'll give more feedback when I get home from work. I envision this class to be like the implementation layer for each DVD/rom/game interface. There should probably also be a top level "core" interface that sits above that, I'll have to think about it more though.

However, another thing that should be unified is heading header data.... e.g. rom/game title, banner, region/country, size, publisher, etc. We should have functions for returning that information individually (or all as a data structure, depending on what neobrain prefers) agnostic of what you are loading.

[neobrain]

I'm extremely critical when judging interfaces, so bear that in mind when reading this post

General comments:
- those virtual methods should be pure ("virtual func() = 0;")
- coding style isn't consistent with gekko's coding style (e.g. parameters are using capital letters)
- no doxygen documentation, yet
- the DVDBase class isn't quite what I had in mind; atm it's more like a file IO abstraction class instead of a interface of requesting an iso's metadata (banner, game name, etc) and reading it. Having a general file IO abstraction class is nice and useful (and should probably done at least at a later point), but wasn't really the point of me asking you guys to create the interface. Maybe I wasn't clear enough when I described my requirements

More specific comments (regarding DVDBase as a file IO abstraction class:
- Load and Close methods aren't really necessary, just make the constructor take the filename and load the file there, close it on destruction (advise stack/non-dynamic allocation if possible so that destructor gets called automatically)
- return values of the methods shouldn't be u32 if possible (using enums for them would be nicer, depending on what return values are possible and if enums are feasible for that)
- seek type should be an enum
- Reset method isn't neccessary, I guess. Sounds like it's bscly just a Seek(0,0,0).
- what's the "u32 filepointer" parameter used for? (please don't tell me it's one of these crippled pointers that are used all over the place in gekko...)

Now the actual interesting stuff, what I'd actually be glad to see would be sth like this:

Code:

enum IsoType { // stupid naming
    ISOTYPE_GCM,
    ISOTYPE_GZC,
    ISOTYPE_DOL,
    ....
};

class IsoMetadata
{
public:
    // default constructor + destructor

    const std::string& GetName();
    const std::vector& GetBannerData();
private:
    // Using strings to avoid having to do manual buffer management!
    std::string name;
    std::vector banner_data;
    // other stuff
};

class File
{
    // add file reading / writing functions here
};

class DVDBase
{
    DVDBase(const char* filename);
    virtual DVDBase();

    // NOTE: We can either make these methods read the stuff they're supposed to return whenever they're called OR just read them once on the first call and then return a non-copyable reference to an instance that is stored in the DVDBase object (in fact, that is probably the better approach).
    IsoMetadata GetMetadata();
    IsoType GetType();
    const File& GetFile();
    // TODO: Add any methods that core needs... dunno what these are since the current boot logic is awkwardingly hard to read and needs big cleanups

private:
    const char* filename;
    File file;
};

Fwiw, it's probably a bad idea to expose the internal file instance in DVDBase via GetFile since that isn't iso-type transparent. Maybe you remember me mentioning that, Dolphin has two nice interfaces: one for "interpreting" ISO data and exposing files to core etc and one for actually reading ISO data (e.g. compressed ISOs can be decompressed on the fly; The actual iso loader can use this interface then to read any iso instead of having to duplicate the gcm loading code for all kinds of images).

Anyway. We also have the option of just using Dolphin's dvd loading code. Since there's no real point in duplicating work here, that might be our best option indeed.

[Shonumi]

(04-20-2012 10:58 PM)neobrain Wrote:  I'm extremely critical when judging interfaces, so bear that in mind when reading this post

Be as critical as you want. You're the one who needs the interface to be as good as possible.

(04-20-2012 10:58 PM)neobrain Wrote:  General comments:
- those virtual methods should be pure ("virtual func() = 0;")
- coding style isn't consistent with gekko's coding style (e.g. parameters are using capital letters)
- no doxygen documentation, yet
- the DVDBase class isn't quite what I had in mind; atm it's more like a file IO abstraction class instead of a interface of requesting an iso's metadata (banner, game name, etc) and reading it. Having a general file IO abstraction class is nice and useful (and should probably done at least at a later point), but wasn't really the point of me asking you guys to create the interface. Maybe I wasn't clear enough when I described my requirements

Cool, I wasn't sure if they needed to be pure virtual functions; I was basing it off of input_common. The coding style is copy+paste from the original DVD loading code (which is old, I assume, hence the inconsistency), and I removed the comments/documentation because I'm in WIP mode right now , just trying to see if the code would actually work. Don't worry, I'll be sure to change all that before I ever commit anything.

Yeah, DVDBase doesn't do anything that the functions in realdvd.h don't already handle. Mainly, I wanted a detailed response like this from you guys before I went on.

(04-20-2012 10:58 PM)neobrain Wrote:  More specific comments (regarding DVDBase as a file IO abstraction class:
- Load and Close methods aren't really necessary, just make the constructor take the filename and load the file there, close it on destruction (advise stack/non-dynamic allocation if possible so that destructor gets called automatically)
- return values of the methods shouldn't be u32 if possible (using enums for them would be nicer, depending on what return values are possible and if enums are feasible for that)
- seek type should be an enum
- Reset method isn't neccessary, I guess. Sounds like it's bscly just a Seek(0,0,0).
- what's the "u32 filepointer" parameter used for? (please don't tell me it's one of these crippled pointers that are used all over the place in gekko...)

Ah, I figured the Load()/Close() would be useless . Reset() too is useless, but not for the reasons you would expect. All it does is reset some function pointers, but we aren't using them anymore. Like I said, I was just doing copy+paste to get it running initially. I'll check out the return values to see how I should setup the enums. About the u32 filepointer, I think it's just used here to determine the read level of the DVD. It's checked to see if it matches REALDVD_LOWLEVEL. Or something like that, here's the comment from the original code:

Code:

//a unique ID incase we wish to read the DVD directly instead of a file from the dvd
//sending this ID to seek or read adjusts the read of the low level data
//sending this to the close function will terminate and cleanup the area controlling
//the RealDVD calls
#define REALDVD_LOWLEVEL        0xFFFFFFFF

(04-20-2012 10:58 PM)neobrain Wrote:  Now the actual interesting stuff, what I'd actually be glad to see would be sth like this:

Code:

enum IsoType { // stupid naming
    ISOTYPE_GCM,
    ISOTYPE_GZC,
    ISOTYPE_DOL,
    ....
};

class IsoMetadata
{
public:
    // default constructor + destructor

    const std::string& GetName();
    const std::vector& GetBannerData();
private:
    // Using strings to avoid having to do manual buffer management!
    std::string name;
    std::vector banner_data;
    // other stuff
};

class File
{
    // add file reading / writing functions here
};

class DVDBase
{
    DVDBase(const char* filename);
    virtual DVDBase();

    // NOTE: We can either make these methods read the stuff they're supposed to return whenever they're called OR just read them once on the first call and then return a non-copyable reference to an instance that is stored in the DVDBase object (in fact, that is probably the better approach).
    IsoMetadata GetMetadata();
    IsoType GetType();
    const File& GetFile();
    // TODO: Add any methods that core needs... dunno what these are since the current boot logic is awkwardingly hard to read and needs big cleanups

private:
    const char* filename;
    File file;
};

Fwiw, it's probably a bad idea to expose the internal file instance in DVDBase via GetFile since that isn't iso-type transparent. Maybe you remember me mentioning that, Dolphin has two nice interfaces: one for "interpreting" ISO data and exposing files to core etc and one for actually reading ISO data (e.g. compressed ISOs can be decompressed on the fly; The actual iso loader can use this interface then to read any iso instead of having to duplicate the gcm loading code for all kinds of images).

Anyway. We also have the option of just using Dolphin's dvd loading code. Since there's no real point in duplicating work here, that might be our best option indeed.

Great, this is exactly the kind of direction I needed. If everyone feels that using Dolphin's loading code would be the best option, I see no real reason to disagree. Any other thoughts?

[ShizZy]

Can we do it better and cleaner than Dolphin's? I don't mind modelling our interface off of theirs, but then we should at least make our code cleaner, more readable, simpler, and correct any possible flaws. If something is pretty generic like just a "load GCZ" function, I don't see a point of reinventing the wheel there. However, even that should still probably be cleaned up to match our style and interface. I would say define our own top level interface and abstraction layer that best suits our needs, and then perhaps recycle some of Dolphin's generic code if that makes sense.

[neobrain]

The only things I can think of right now is a) banner loading needs to be handled slightly differently b) file loading is not stable enough (probably crashes if you throw random files at it) c) there's IIRC no function to determine file type of an image. There might be other flaws as well, but those should be merely implementation related.

[Shonumi]

(04-21-2012 03:40 AM)neobrain Wrote:  The only things I can think of right now is a) banner loading needs to be handled slightly differently b) file loading is not stable enough (probably crashes if you throw random files at it) c) there's IIRC no function to determine file type of an image. There might be other flaws as well, but those should be merely implementation related.

I think b and c kinda go hand-in-hand. Right now, the the loading code simply checks the extension of the file, so there's no real verification if you are in fact loading an ISO (and not a ELF file renamed with a .iso extension). As is, the situation isn't optimal. However, verifying the image before loading it could also take care of finding out its type.

Any specifics on how improving banner loading? I haven't looked at that code in-depth yet, unfortunately.

[neobrain]

Nothing problematic, dolphin code is just storing a wxImage instead of the raw banner data or sth IIRC.

[ShizZy]

Well I can't imagine we'd want it to store a Qt image.... since that introduces a big dependency to the core. So what's fine with just a raw image?

Perhaps we could add support for other formats (Wii supports multiple image formats for banners, right?) but that is certainly second priority for the time being.

[neobrain]

"So what's fine with just a raw image?" <-- what?

That was just what I said, we should change it to use a u8 vector instead of wxImage.