Closed
Bug 464180
Opened 14 years ago
Closed 14 years ago
link error when building nsWaveDecoder on wince
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: blassey, Assigned: wolfe)
References
Details
(Keywords: mobile)
Attachments
(1 file, 2 obsolete files)
5.11 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
_FNan is undefined
Reporter | ||
Comment 1•14 years ago
|
||
Attachment #347463 -
Flags: review?
Reporter | ||
Updated•14 years ago
|
Attachment #347463 -
Flags: review? → review?(kinetik)
Comment 2•14 years ago
|
||
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.
Updated•14 years ago
|
Component: Content → Video/Audio
QA Contact: content → video.audio
Version: unspecified → Trunk
Comment 3•14 years ago
|
||
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•14 years ago
|
||
roc, chris, ideas?
Comment 5•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
fwiw, prints 0.000000
Comment 9•14 years ago
|
||
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•14 years ago
|
||
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
Comment 11•14 years ago
|
||
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•14 years ago
|
||
john, you said you were looking at this. any progress?
Assignee | ||
Comment 13•14 years ago
|
||
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•14 years ago
|
||
lets go with your static library approach. can you toss a patch up?
Assignee | ||
Comment 15•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #352436 -
Flags: review?(bugmail) → review-
Assignee | ||
Comment 16•14 years ago
|
||
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.
Assignee | ||
Comment 17•14 years ago
|
||
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 18•14 years ago
|
||
Comment on attachment 352482 [details] [diff] [review] v3.0 patch ted, can you take a look?
Attachment #352482 -
Flags: review?(bugmail) → review?(ted.mielczarek)
Updated•14 years ago
|
Attachment #352482 -
Flags: review?(ted.mielczarek) → review+
Comment 19•14 years ago
|
||
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.
Comment 20•14 years ago
|
||
fennec on wince will not build without this change.
Flags: blocking1.9.1?
Flags: blocking-fennec1.0?
Comment 21•14 years ago
|
||
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
Reporter | ||
Comment 22•14 years ago
|
||
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?
Assignee | ||
Comment 23•14 years ago
|
||
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.
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-
Reporter | ||
Comment 26•14 years ago
|
||
bug 474737
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
tracking-fennec: --- → ?
Flags: blocking-fennec1.0?
Updated•9 years ago
|
tracking-fennec: ? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•