link error when building nsWaveDecoder on wince

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: blassey, Assigned: wolfe)

Tracking

({mobile})

Trunk
ARM
Windows CE
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

_FNan is undefined
Posted patch implements symbol in source (obsolete) — Splinter Review
Attachment #347463 - Flags: review?
Attachment #347463 - Flags: review? → review?(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.
Component: Content → Video/Audio
QA Contact: content → video.audio
Version: unspecified → Trunk
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?
roc, chris, ideas?
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.
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.
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.
fwiw, prints 0.000000
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?
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.
Assignee: nobody → doug.turner
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.
john, you said you were looking at this.  any progress?
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.
lets go with your static library approach.  can you toss a patch up?
Posted patch v2.0 patch (obsolete) — Splinter Review
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.
Attachment #347463 - Attachment is obsolete: true
Attachment #352436 - Flags: review?(bugmail)
Attachment #347463 - Flags: review?(kinetik)
Attachment #352436 - Flags: review?(bugmail) → review-
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.
Posted patch v3.0 patchSplinter Review
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.
Attachment #352436 - Attachment is obsolete: true
Attachment #352482 - Flags: review?(bugmail)
Comment on attachment 352482 [details] [diff] [review]
v3.0 patch

ted, can you take a look?
Attachment #352482 - Flags: review?(bugmail) → review?(ted.mielczarek)
Attachment #352482 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 352482 [details] [diff] [review]
v3.0 patch

+ * The Initial Developer of the Original Code is Doug Turner <dougt@meer.net>.

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.
fennec on wince will not build without this change.
Flags: blocking1.9.1?
Flags: blocking-fennec1.0?
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
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?
Depends on: 472165
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.
-> wolfe@lobo.us
Assignee: doug.turner → wolfe
Why should this block 1.9.1? We're shipping Fennec off 1.9.2 aren't we?
Flags: blocking1.9.1? → blocking1.9.1+
Flags: blocking1.9.1+ → blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1-
bug 474737
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
tracking-fennec: --- → ?
Flags: blocking-fennec1.0?
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.