464180 - link error when building nsWaveDecoder on wince

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

_FNan is undefined

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: implements symbol in source

Comment 2

[Matthew Gregan [:kinetik]]

I guess this is okay as a workaround, but it's pretty ugly.  Please add a comment explaining why it's necessary.  On Win32/Vista, it looks like _FNan is exported from msvcp60.dll.  Does that exist in WinCE?  If I understand correctly, that DLL is (part of) the C++ CRT, so we shouldn't have to do anything magic to link against it.

nsOggDecoder and any other decoder backends need to return a NaN in certain cases as required by the HTML5 video/audio spec.  It's possible to compile without Wave decoder support, so if we take this fix I think it needs to be made somewhere else in the tree so that the other backends can benefit from the same workaround.

Comment 3

[Matthew Gregan [:kinetik]]

For what it's worth, the only other code in the tree that needs to generate a NaN (the JS engine) does so by synthesizing one at runtime and storing the result in the js_NaN private global variable.  Given that it's private and a double (where we need a float), it's probably of no use to us.

Perhaps a more portable approach would be to synthesize a NaN doing the following, which is suggested in the C standard:

  float gNaN = strtof("NAN", NULL);

Again, this would need to be exported somewhere that the backend decoders could all access it.  Perhaps nsMediaDecoder.cpp?

Comment 4

[Doug Turner (:dougt)]

roc, chris, ideas?

Comment 5

[Matthew Gregan [:kinetik]]

How about someone with a WinCE build environment test:

  float gNaN = strtof("NAN", NULL);

and either update the existing patch or let me know it works so I can update the patch?  It can be made a static global in nsMediaDecoper.cpp initialized the first time an nsMediaDecoder is instantiated.  nsWaveDecoder.cpp and nsOggDecoder.cpp will need to be updated to use the global, and it still needs some comments added explaining why we can't use numeric_limits<float>::quiet_NaN() on such utterly broken platforms.

Comment 6

[Doug Turner (:dougt)]

strtof doesn't exist in windows mobile.  Maybe we can do something similar to what you are suggesting, but just force the value of NaN to the known value on wince?

Other projects seem to also define _FNAM in terms similar to what brad has done (although, i am not sure if we need all of those lines).

what is most disturbing is that the wince headers define quite_NaN(), but do not export _fNaN.

Comment 7

[Matthew Gregan [:kinetik]]

Oh, that's not too surprising, strtof is C99.  Somehow I didn't notice that, otherwise I wouldn't even have suggested it, knowing the state of C99 support in the wild.  Sorry.

Can you confirm that strtod is available? It should be, that's been around since C89.  We can use that to generate a NaN and then let automatic conversion deal with double->float.  As far as I can tell, it should be a valid double->float conversion.

Just to be sure this approach is going to work, would you be able to compile and run the following test program on WinCE?

#include <stdio.h>
#include <stdlib.h>

int
main(int argc, char * argv[])
{
	float nan = (float) strtod("NAN", NULL);
	printf("%f\n", nan);
	return 0;
}

It should compile and link fine (unless it ends up trying to reference _FNan again), and print "nan" (or similar) when executed.

Comment 8

[Doug Turner (:dougt)]

fwiw, prints 0.000000

Comment 9

[Matthew Gregan [:kinetik]]

Thanks.  It's possible that strtod works and WinCE's printf doesn't deal with NaN correctly, but I don't want to waste too much time on this.  Let's assume it's totally broken and go with Brad's original fix.

Is it possible to put the fix in the shunt DLL instead?  That way it's available on WinCE, relatively obvious where to find it should the real _FNan appear in a later WinCE release, and doesn't uglify the code in nsWaveDecoder.cpp?

Comment 10

[Doug Turner (:dougt)]

yes, i think that is fair.  It would be great if the shunt could simply export the symbol and not have to #define anything publicly.  If we do, then your code might have to #include ymath or something so that the shunt gets hooked in correctly.

I will take a look later tonight/tomorrow.

Comment 11

[Doug Turner (:dougt)]

odd.  i can't seem to export a union from the shunt dll.  It looks like it is there when I view it in depends, but the linker can not find it when linking.

Comment 12

[Doug Turner (:dougt)]

john, you said you were looking at this.  any progress?

Comment 13

[John Wolfe (:wolfe)]

Doug, unfortunately, the current mozce_shunt.dll is an external DLL.

The way that _FNan is defined is as a existing-within-a-statically-linked-lib

In order to not have extra WinCE code for this issue, we would have to create a very small LIB file to be linked in with executables needing _FNan.

I understood that this extra lib linkage was too much to add into the WinCE build.

Or am I mistaken about this understanding?  If so, I have a patch that does exactly what we need to create a statically-linked LIB file and adds it to links.

The downside of this extra statically-linked lib is a little extra space for debug DLLs and EXEs.  

Release builds will optimize out the _FNan unreferenced variable when stripping down DLLs and EXEs - so this fix will not have a large effect on Release builds.

Comment 14

[Doug Turner (:dougt)]

lets go with your static library approach.  can you toss a patch up?

Comment 15

[John Wolfe (:wolfe)]

Attachment: v2.0 patch

Moves symbol generation into a created-header file (ymath.h) for WinCE builds.

Removes need to change source files by adding #ifdef WINCE and code into specific C files (such as content/media/video/src/nsWaveDecoder.cpp).

Review requested by Brad Lassey for produced "ymath.h" header file

Review needed for changes to configure.in file post-Brad-Lassey r+ granting.

Comment 16

[John Wolfe (:wolfe)]

Comment on attachment 352436 [details] [diff] [review]
v2.0 patch

Extra lines in configure.in were not to Brad's liking.  Shuffling around header files to reduce "configure.in" footprint.

Comment 17

[John Wolfe (:wolfe)]

Attachment: v3.0 patch

Paired down involvement of TOPSRCDIR/configure.in, as per blassey's suggestions.
Now TOPSRCDIR/configure.in just sets a single line into two specific header files, which are in turn loaded by the two header files listed below.

This reduces the footprint of this patch on the configure and configure.in files, while still keeping the dynamic reconfiguration of SDK paths for include files.

Added two header files into TOPSRCDIR/build/wince/shunt/include:

windows.h - hard-coded to include a mozce_windows_actual_incl.h header file created by TOPSRCDIR/configure, as well as some code to get rid of compiler warnings about redefinition of GetProcAddress

ymath.h - hard-coded new header file containing fix for _FNan link errors, as well as some code to bypass compiler errors about redefining _FNan with a different linkage.

Comment 18

[Doug Turner (:dougt)]

Comment on attachment 352482 [details] [diff] [review]
v3.0 patch

ted, can you take a look?

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 352482 [details] [diff] [review]
v3.0 patch

+ * The Initial Developer of the Original Code is Doug Turner <[e-mail]>.

I believe this is supposed to read "The Mozilla Foundation".

r=me on the build bits, I have no idea what you're actually doing in that ymath header.

Comment 20

[Doug Turner (:dougt)]

fennec on wince will not build without this change.

Comment 21

[Doug Turner (:dougt)]

lets do this:

1) make windows.h -> window.hs.in
2) create some configure love to process windows.h.in and produce windows.h in the object directory
3) fix up the tools to look at the windows.h in the object directory
4) do the same thing for ymath.h

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

while we're doing this, the tools should be put in $(objdir)/dist/bin instead of build/wince/tools/bin.

and to clarify windows.h and ymath.h should be in dist/include/shunt/ I assume?

Comment 23

[John Wolfe (:wolfe)]

This bug is triggering a revamping of the build tools and the build sequence to allow the WinCE shunt tools to more closely match the OBJDIR model.  

I marked this bug as dependent upon Bug 472165 - WinMobile Build Requires Tools In PATH Variable.

Comment 24

[Doug Turner (:dougt)]

-> [e-mail]

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why should this block 1.9.1? We're shipping Fennec off 1.9.2 aren't we?

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

bug 474737