Re: [Audacity-devel] Second Exports Patch

[Stephen P. <sg...@ma...>]

Hi Martyn,
Thanks for your careful consideration of this - my replies are below.

Message: 4
>
Date: Sun, 12 Jan 2014 23:59:41 +0000
> From: Martyn Shaw <mar...@gm...>
> Subject: Re: [Audacity-devel] Second Exports Patch
> To: aud...@li...
> Message-ID: <52D...@gm...>
> Content-Type: text/plain; charset=ISO-8859-1; format=flowed
>
> Hi
>

> On 11/01/2014 18:29, Stephen G. Parry wrote:
> > @Martyn
> > If you look at the example for WX_DECLARE_OBJARRAY on the reference
> > page: http://docs.wxwidgets.org/trunk/dynarray_8h.html you will see
> > the comment says:
> >
> > class MyClass;
> > WX_DECLARE_OBJARRAY
> > <
> http://docs.wxwidgets.org/trunk/dynarray_8h.html#a515e9265b474d8a152ec3ec42179abde
> >(MyClass,
> > wxArrayOfMyClass); // note: not "MyClass *"!
> >
> > That warning is then repeated for the other macros. That's all there
> > is. Thin I know, but can we safely ignore it?
>
> There is
> http://docs.wxwidgets.org/2.8/wx_wxarray.html#wxdeclareobjarray
> where it says "you should use the _PTR versions of the macros", so
> maybe that's a clue?
>
> Also
> http://msdn.microsoft.com/en-us/library/esew7y1w.aspx
> seems to be saying that you shouldn't export things to dll that could
> muck you up or mess with you.  Maybe that's pointers.
>
> Like I said, I'm no expert on this stuff.  But I'm all for improving
> the plug-in dll stuff; I just wish I had the expertise!
>
>
OK I will review the use of the _PTR macros - the code has been there a
while, but you never know what unresolved bugs might be down to things like
this.

> @James
> > Yes, all I am doing is exposing more of the API and I am happy with
> > the usual contractual caveats.
>
> But warning are saying (perhaps) that we are exposing too much.
>

The warnings go away if you expose MORE of the API, not less...


>   I have to recompile my plug-in for each
> > rev of audacity anyhow, so there it is.
>
> 'Unfortunately' there is a known way around that, it is not a good
> system.  Aurora just read the version from the prefs.
>

Having to review and recompile each version is an acceptable shortcoming
for us, but not if we have re-rip and re-hack large chunks, which is where
we are without the exports I've proposed.


>
> > The warning I've suppressed in my first patch is C4251
> >
> > 1>c:\development\audacity-trunk\src\tags.h(112) : warning C4251:
> > 'Tags::mIter' : class 'TagMap_wxImplementation_HashTable::iterator'
> > needs to have dll-interface to be used by clients of class 'Tags'
> > 1>        c:\development\audacity-trunk\src\tags.h(53) : see
> > declaration of 'TagMap_wxImplementation_HashTable::iterator'
> > 1>c:\development\audacity-trunk\src\tags.h(113) : warning C4251:
> > 'Tags::mXref' : class 'TagMap' needs to have dll-interface to be used
> > by clients of class 'Tags'
> > 1>        c:\development\audacity-trunk\src\tags.h(53) : see
> > declaration of 'TagMap'
> > 1>c:\development\audacity-trunk\src\tags.h(114) : warning C4251:
> > 'Tags::mMap' : class 'TagMap' needs to have dll-interface to be used
> > by clients of class 'Tags'
> > 1>        c:\development\audacity-trunk\src\tags.h(53) : see
> > declaration of 'TagMap'
> >
> > The compiler assumes that because we have exported Tags, we want to
> > export all the classes that define its members.
>
> I'm not sure that that is the correct assessment of the message (see
> above).  But again, I'm not at expert with this.
>

I have checked different scenarios and am deep in testing at the minute. So
far Audacity is 100% stable with the exports in place. I am still reworking
our tag handling code to use the exports, and that will give a greater
level of certainty. We have some memory leaks, but these seem to be from an
unrelated problem in Audacity trunk.


>
>   We don't, especially
> > as the members in question (mIter, mXref and mMap) are private. I've
> > added an explanatory comment. New patch attached, replacing BOTH
> > previous ones.
> >
> > If we went for the more limited export approach on the second patch to
> > class Exporter and its children,  then it would need the same pragma
> > and comments, but if we are happy with the full patch exposing the
> > three classes (FileFormatInfo, ExporterPlugin and Exporter) and the
> > supporting arrays, then no pragmas are needed.
>
> I currently think that we should make the minimum changes needed to
> support easy and safe support of your project (which I see at
> https://github.com/sgparry/eutychus) (thanks from to OS community!).
> My problem is that I don't know what they are, and don't have the
> knowledge to approve them.
>

The patches I have proposed are a tiny proportion of the code base. With
the exception of the SaveProject and OpenProject script commands I put in
in patch 3 (the rest of that patch, the new SaveAs overload in class
Project, is essential), they are the minimum I need to support our code -
they are just enough to allow me to dispense with the ugly hacks I have had
to do and no more. I have specifically avoided changing any existing
functionality in Audacity by adding not changing. The new DLL entry points
should only conceivably destabilize Audacity if someone actually uses them
- which is our lookout as anything at that level is experimental anyhow.

>
> > Are you OK to commit this please?
>
> I am not, at this point.  Sorry.  I really am.  I want you to be able
> to very easily develop dlls that do great stuff.  If others are
> reading this, please approach us earlier in your development cycle.
>

So where to we go from here? Our live code is working great with the
current release of audacity, but to get there I have had to make so many
stupid ugly hacks - e.g. copying in code from Audacity wholesale, cutting
out references to unneeded code and compiling that into our plugin. When
the next release appears I am going to potentially have to redo all that
work again.

We carefully considered all alternatives before embarking on this
development, but:
- Manually using Audacity without the plug-in increases the work involved
on a weekly basis by 300% and introduces errors at every stage.
- The scripting interface simply does not do what we need. There is no
access to meta-data and no way of automating Save As. Plus using it would
mean having a separate UI, which is counter-intuitive for novice users.
- We could fork Audacity, but that has been tried by others in the past and
proved a vast waste of effort.
- We need both automation and interaction so things like sox are no use.
- We need lossless recording for archival purposes.
- No other Open Source product does what Audacity does.
Are they any possibilities I've missed that you can suggest?

I should point out that I am willing to commit to help long term on the
extensibility features in Audacity. I am a C++ developer with 20 years
industry experience on Windows, GNU/Linux and Unix. Plugin APIs, scripting
interfaces and such are very much my bag and I would very much like to help
in turning Audacity into a fully modular, extensible and scriptable beast.

Thanks again
Stephen Parry


> TTFN
> Martyn
>
> > Thanks
> > Stephen Parry
> >
> >
> > On 10/01/2014 02:31, aud...@li... wrote:
> >> Message: 4 Date: Fri, 10 Jan 2014 00:29:36 +0000 From: Martyn Shaw
> >> <mar...@gm...> Subject: Re: [Audacity-devel] Second
> >> Exports Patch To: aud...@li... Message-ID:
> >> <52C...@gm...> Content-Type: text/plain;
> >> charset=ISO-8859-1; format=flowed On 09/01/2014 22:14, James Crook
> >> wrote:
> >>> Stephen -
> >>>
> >>> I think all these patches are simply exposing more of the Audacity API?
> >>> Is that right?
> >> That looks right to me, but I'm no expert on this stuff.
> >>
> >>> I'm totally fine with that.  Just we make no commitment to preserve
> that
> >>> API in
> >>> future versions.  We're just not yet at that stage.  If it's useful, I
> >>> say make it available.
> >> James, I think that you are the expert with this part of the code.
> >> Please commit it if you think it's safe, or comment if you won't.  And
> >> thanks for responding on this!
> >>
> >>> Silencing warnings though - what specific warning?  If it really is
> >>> needed, I think  it
> >>> should have a comment to say what the warning silences, and why it is
> safe.
> >> One of the patches has '4251'.
> >>
> >> TTFN
> >> Martyn
> >>
> >>> "WX docs specifically discourage this practice"
> >>>
> >>> Please give a link to the url that says this.  I'd like to understand
> >>> why they think it a
> >>> bad idea.
> >>>
> >>> --james.
> >>>
> >>>
> >>>
> >>> On 09/01/2014 02:27, Stephen G. Parry wrote:
> >>>> Hi All,
> >>>>
> >>>> This is my second small proposed patch for audacity to ease the
> writing
> >>>> of some UI in-process plug-ins.
> >>>>
> >>>> My Audacity plug-in finishes its work by exporting the project in
> >>>> different formats, FLAC for archival purposes and MP3 for upload to
> our
> >>>> web-site.
> >>>>
> >>>> Doing this from within the UI is not easy. The only exported entry
> point
> >>>> is via the scripting interface, which requires executing the code in a
> >>>> worker thread, else the whole app stalls (more on this in a later
> post).
> >>>> This would be much easier if the exporter was a DLL export. Attached
> is
> >>>> a patch to do just that.
> >>>>
> >>>> CAVEAT: this patch is open to discussion on two points. Firstly
> because
> >>>> of I have gone beyond exporting the Exporter class; I have also
> exported
> >>>> the ExportPlugin and the FormatInfo classes, paving the way for
> someone
> >>>> to start coding DLL based export plugins. (It also silences some
> >>>> compiler warnings). Some may consider this a step too far. Secondly,
> >>>> those exports rely on a the WX macro
> WX_DECLARE_USER_EXPORTED_OBJARRAY;
> >>>> the documentation on this macro is both incomplete and inaccurate.
> This
> >>>> raises portability / maintainability issues.
> >>>>
> >>>> If the community feels this aspect of the patch is not worth having
> then
> >>>> I will happily cut the patch back to just the Exporter class and
> pragma
> >>>> out the warnings. The code should still function perfectly for
> exporting
> >>>> the normal bundled formats.
> >>>>
> >>>> A final note: ExportPluginArray is based on a pointer to ExportPlugin.
> >>>> The WX docs specifically discourage this practice. Any thoughts?
> >>>>
> >>>> Thanks
> >>>> Stephen Parry
> >>>
>