#128 Increase usefulness of the generic mic.cpp file

open
None
5
2011-06-18
2011-06-18
rogerman
No

The generic mic.cpp file doesn't have to be a dumb stub file -- it can actually do useful things. This patch makes it useful. This patch was made to support the future Cocoa port, which will need to generate fake mic noise in the next release.

As a side benefit, any existing ports that take advantage of the generic mic file (such as any port that uses the ctrlssdl.cpp file) will instantly gain the ability to generate working fake mic noise without additional changes.

Discussion

  • Magliocchetti Riccardo

    The intent is right, could you please respin without all that boilerplate documentation for functions? i.e. Mic_DeInit() is already self explanatory and in my opinion does not need a line of comment. Then do we really need that many functions?
    Does micInternalNoiseSampleBuffer need to be exported in the .h? i think it does not and moved to mic.cpp can have a shorter name too.
    Also could you please use K&R style for conditionals (braces in the same line)?

     
  • Magliocchetti Riccardo

    • assigned_to: nobody --> riccardom
     
  • rogerman

    rogerman - 2011-06-18

    Okay, let me work on this for a bit...

    Boilerplate documentation - Removed.

    Need for functions - I would think so, since more functions would add more flexibility to the code. In fact, there's a bunch of missing functions that should be in there, but I didn't implement yet. (For example, missing functions for reading/writing a block of samples. Currently, the code can only read/write one sample at a time.)

    micInternalNoiseSampleBuffer in .h - You're right. This was a bit of old stuff I did when I first started playing around with the code. I included it in the Mic_GenerateInternalNoiseSample() function and cleaned up the code a bit.

    K&R style conditionals - Reworked.

    New patch is attached.

     
  • Alex

    Alex - 2011-06-20

    Might want to put parens around MIC_BUFFER_SIZE macro.

     
  • rogerman

    rogerman - 2011-06-20

    Oh yeah, missed that one. Thanks, scorp007!

     
  • Magliocchetti Riccardo

    I'm still failing to see why we need so many exported interfaces, why would one want to poke with the Mic_FillBufferWith* functions if they are managed by a config option? The same goes for Mic_Generate*Sample functions in my opinion. Does the cocoa frontend needs all this stuff?

     
  • rogerman

    rogerman - 2011-07-23

    All the functions that you mentioned exist for the purposes of convenience and abstraction. This isn't really a Cocoa-specific thing -- it's just the right thing to do from a design standpoint. It gives the individual ports more freedom with how to implement the mic features.

     
  • Magliocchetti Riccardo

    Let me disagree with that, i see no value on adding interfaces that export implementation details of the fake mic. mic.h is the common api and should not contain backend specific stuff.

     
  • rogerman

    rogerman - 2011-07-23

    Then the API is insufficient for DeSmuME's existing feature set. For example, a buffer is a generic requirement for storing mic samples. The functions related to the internal noise and white noise sample generation are also generic requirements.

    Okay then, it seems like we have a difference of opinion on how much functionality an API should expose, so we'll play it your way. As it stands, the individual ports should implement their own mic handling like the Windows port -- don't use the generic mic.cpp file, let individual ports define their own source file, and duplicate generic functionality for each and every port.

    I believe that the best solution is to not apply this patch and close out this issue. And let's just leave it at that.

     
  • Magliocchetti Riccardo

    Let me clarify that the changes in mic.cpp are completely fine so i'll apply them. I don't mean to remove the functionality i just don't want to export them and all the stuff to change the fake mic behaviour in mic.h. Please also take into account that it was exporting declarations of functions that are not defined in other backends mic_alsa.cpp and mic_openal.cpp.
    I completely agree that this code should be shared between ports and in fact the gtk and cli stuff use mic.cpp as fake mic :) Thanks!

     
  • Magliocchetti Riccardo

    I've attached a preliminary patch of what i mean in practice, compile tested only. It also consolidate alll the Mic_FillBuffer* functions in DoNoise. What do you think?

     
  • rogerman

    rogerman - 2011-07-24

    My code should have been completely backwards compatible with all ports (except Windows, which uses its own source file). I took great care in designing it in such a way that none of the ports would break, while making the code base a lot more flexible at the same time.

    About Mic_DoNoise() -- it's really a convenience function at heart, so I supported it due to its simplicity, and also for backwards compatibility. But a port shouldn't be required to use it because there are better ways of implementing mic noise. For example, by using the Mic_FillBuffer* functions, someone could decide in their port to fill the buffer with samples in advance. Or, they could fill the samples on a separate thread. Both of these techniques would make it possible to perform effects preprocessing before sending the final sample data to DeSmuME.

    But the big glaring omission is the missing function pointer in Mic_ReadSample(). Of all the changes that should be made to mic handling, this is definitely the most important one! There needs to be a way for ports to override the default sample reading behavior. All mic sample data is eventually read at _MMU_ARM7_write16() in MMU.cpp:4053 using a direct call to the C-style function Mic_ReadSample(). Because of this design, there is no way to change how Mic_ReadSample() returns a sample without replacing the function entirely. This is what we currently do with the Windows port, and also mic_alsa.cpp and mic_openal.cpp. But with a function pointer in Mic_ReadSample(), the individual ports would now have the option of using the generic code base for the common stuff, and then do port-specific streaming of sample data back to _MMU_ARM7_write16(). This would, hopefully, eliminate the feature gaps between ports.

    The best case would be if we encapsulated the mic handling code within a C++ object and making Mic_ReadSample() a virtual method, but I didn't want to refactor that far in a single patch. Now, this is just one example of this type of non-portable situation in DeSmuME. There are a lot more examples, but the mic code is the easiest to understand and fix. So the reason all the accessor functions existed was because the code was being prepped to encapsulate into a C++ object someday. But regardless, accessors are just the right thing to do in general.

    Sorry for the lengthy reply, but that code took quite a bit of effort to design, and I felt the need to justify the usage of all those functions. In short, I think your patch changes are fine if we're going for short-term simplicity vs long-term flexibility. But we absolutely do need the function pointer in Mic_ReadSample(). Otherwise, each port will need to reinvent the wheel each time they want to implement mic functionality.

    One more thing... check this code --

    if (!noise) {
    + generator = &Mic_GenerateNullSample;
    + } if (CommonSettings.micMode == TCommonSettings::InternalNoise) {
    + generator = &Mic_GenerateInternalNoiseSample;
    + } else if (CommonSettings.micMode == TCommonSettings::Random) {
    + generator = &Mic_GenerateInternalNoiseSample;
    + }

    I think you want Mic_GenerateWhiteNoiseSample for the third conditional block...?

     
  • rogerman

    rogerman - 2011-07-24

    One other thing, I don't understand why it's wrong to have unimplemented function prototypes in mic.h. Is it causing compile warnings/errors? I've tested this on GCC 4.2 on OS X and VS2008 on Windows, and everything works just fine with no warnings/errors.

     
  • Magliocchetti Riccardo

    First of all sorry for taking so long to respond to your message.
    Then in the mean time we had a new submission for a new mic interface, see patches at issue #3406624. I hope to clarify the problems i see with your approach by looking at his patches. As you can he of course suffers from the Not Invented Here syndrome :) and add new files instead of replacing the old one, anyway he gets right one thing: he create a base class with a _few_ public methods each different backend can override. So this is what i'm talking about and you seem to acknowledge as a subsequent move to do. I'd say just rework the mic in a sane way now instead of doing intermediate steps which adds in my opinion only more confusion.
    My patch was an attempt to merge first all of the stuff i think will be keeped without breaking backward compatibility before going fully OO. So what do you think if apply my patch (after i actually tried it possibly :) and start discussing a proper class layout? Thanks.

     
  • rogerman

    rogerman - 2011-09-18

    No worries -- I've had time to work on other stuff... :-p

    And sure, the C++ OO approach will definitely work, but I still stand by what I did given the limitations of coding in straight C. What I think this disagreement is is philosophical differences to coding more than anything else. We just need to get together, figure out what the intentions are, and what the requirements are. How do we redesign this properly? We'll leave that for a future discussion. I'd be happy to discuss class layout with you!

    Again, I like your patch, and I think that it's perfectly fine for a first step. In reality, the mic code for the working Cocoa port only does noise generation, so this patch would work fine for it. Again, the big limitation is Mic_ReadSample(), specifically the inability to override the Mic_DefaultBufferRead() call. But this can be addressed in a future patch.

    Did you check this out yet? The third conditional block should be &Mic_GenerateWhiteNoiseSample.

    if (!noise) {
    + generator = &Mic_GenerateNullSample;
    + } if (CommonSettings.micMode == TCommonSettings::InternalNoise) {
    + generator = &Mic_GenerateInternalNoiseSample;
    + } else if (CommonSettings.micMode == TCommonSettings::Random) {
    + generator = &Mic_GenerateInternalNoiseSample;
    + }

     
  • Magliocchetti Riccardo

    Ok applied the preliminary patch with the issue you noticed fixed. Attached a still work in progress patch that should show what i have in mind, what do you think?

     
  • Magliocchetti Riccardo

    The latest patch compiles and does not make the gtk frontend crash. Don't know it works though. Anyway the important bits are that i've removed some duplicate (at least in my opinion :) stuff from mic.cpp which is what we disagree on. This is because i think mic.cpp should run only when !physical mic mode. I've included the configure and mic_alsa.cpp changes to make it clear that physical implementation should be separated code. What do you think? The switch from one backend to another should be a frontend choice and i suppose i'll use something like PhysicalMicList in mic.h on gtk and cli frontends.

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks