User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:22.0) Gecko/20130329 Firefox/22.0 Build ID: 20130329083344 Steps to reproduce: Logged into my google account. Actual results: After I logged in on my google account, I wanted to manage my musical collection. But, it doesn't display anything. Progress bar is not display, and page remains empty. Expected results: Displaying all my music. Looking at error console, I saw a lot of warning starting with "Warning: Unknown pseudo-class or pseudo-element '-webkit-scrollbar-track'. Ruleset ignored due to bad selector." I don't know for how much display had been broken.

Forget to add : I can see that on both homemade builds and "official" nightlies. My last homemade build is based on http://hg.mozilla.org/mozilla-central/rev/8693d1d4c86d

After doing some research, using linux 64 bits builds, I found the last working build and the first broken one. 17 march : http://hg.mozilla.org/mozilla-central/rev/0b052daa913c : ok 18 march : http://hg.mozilla.org/mozilla-central/rev/b03bb3ce8cee : broken

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b052daa913c&tochange=b03bb3ce8cee I don't see any style system changes in that range. There's a lot of DOM changes though. I think the "Unknown pseudo-class or pseudo-element" is unrelated to the problem. Do you see those warnings also in the build that works?

This needs steps to reproduce, ideally..... ones that someone who has never done any music stuff with a Google account could follow.

(In reply to Mats Palmgren [:mats] from comment #3) > http://hg.mozilla.org/mozilla-central/ > pushloghtml?fromchange=0b052daa913c&tochange=b03bb3ce8cee > > I don't see any style system changes in that range. There's a lot of DOM > changes though. > > I think the "Unknown pseudo-class or pseudo-element" is unrelated to the > problem. Do you see those warnings also in the build that works? Same output indeed :( I will look at error console with a newer build asap and see if there is anything else.

By the way, I only get css error in Error console, like : Warning: Expected color but found 'top'. Error in parsing value for 'background-image'. Declaration dropped. Source File: https://plus.google.com/_/scs/apps-static/_/ss/k=oz.sbw.1h295td2rco7g.L.F4.O/am=AAACAM5BgAAgJAAAgMQh5n4BAAAAAwg/rs=AItRSTMOBrsfZ0J-6qL9aoTRz60C_dklOg Or Timestamp: 29/03/2013 17:29:42 Warning: Unknown property '-moz-box-shadow'. Declaration dropped. Source File: https://plus.google.com/_/scs/apps-static/_/ss/k=oz.sbw.1h295td2rco7g.L.F4.O/am=AAACAM5BgAAgJAAAgMQh5n4BAAAAAwg/rs=AItRSTMOBrsfZ0J-6qL9aoTRz60C_dklOg A lot of declaration dropped also. Any idea ? No error, only warning related to css file :(

See comment 4? Seriously, it's hard to say anything here without having some idea of what the page is doing.

I saw it. So without anyone with a google music account, this bug will be blocked.

Hmm. Well, if you save the page as "web page, complete", does it happen to work locally? If not, are you willing to bisect on tinderbox builds?

DOM seems more likely than Style System given the regression range (comment 3).

(In reply to Boris Zbarsky (:bz) from comment #9) > Hmm. Well, if you save the page as "web page, complete", does it happen to > work locally? Nothing. Same problem. > > If not, are you willing to bisect on tinderbox builds? Just tell me which tinderbox builds to get and how to bisect. I want to see this bug fixed.

> Nothing. Same problem. What I meant was, does the saved version work in the old nightly but fail in the new one? If so, you could attach the saved version here and I could debug. ;) > Just tell me which tinderbox builds to get and how to bisect. Bisect the same way as you do on nightlies. As for which builds, http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux/ or http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64/ depending on which one you want, I assume. And then look for the relevant dates. Note that you might have to go back another day or so because things might have landed on inbound a good bit before the merge to mozilla-central. If you do have time to do that, that would be great!

(In reply to Boris Zbarsky (:bz) from comment #12) > > Nothing. Same problem. > > What I meant was, does the saved version work in the old nightly but fail in > the new one? If so, you could attach the saved version here and I could > debug. ;) I will try to see if there is no personal datas and upload it. > > > Just tell me which tinderbox builds to get and how to bisect. > > Bisect the same way as you do on nightlies. > > As for which builds, > http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla- > inbound-linux/ or > http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla- > inbound-linux64/ depending on which one you want, I assume. And then look > for the relevant dates. Note that you might have to go back another day or > so because things might have landed on inbound a good bit before the merge > to mozilla-central. > > If you do have time to do that, that would be great! Will do this asap tonight (european time).

> I will try to see if there is no personal datas and upload it. Makes sense. Please feel free to email it to me directly or something if you're more comfortable with that.

First of all, builds :) Following publishing date and hours, looks like last working build was based on : http://hg.mozilla.org/integration/mozilla-inbound/rev/3a5f73a4f816 First broken one : http://hg.mozilla.org/integration/mozilla-inbound/rev/a8b4ad85b576 Will add asap datas, as it looks like there is no sensitive personal datas in it ;)

saved page, hoping it could help.

> First of all, builds :) Thanks. The range there is http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3a5f73a4f816&tochange=a8b4ad85b576 If you take a nightly that's broken and set the "media.webaudio.enabled" preference to false, does that fix things?

Verified on my homemade build (10 hours old)... And you got it ! Deactivating webaudio fixes the bug !

And thank you for the files. So listen_fr.js has this bit: this.sh = l; try { typeof webkitAudioContext == CH ? this.sh = new webkitAudioContext : typeof AudioContext == CH && (this.sh = new AudioContext) } catch (b) {} this.XA = l; this.sh && (this.XA = this.sh.createAnalyser()) Pretty sure we don't implement createAnalyzer, so this is going to throw... and pretty sure the site just ends up hiding the exception or something; that sort of thing is all over that script.

You're welcome for the files. So, besides deactivating webaudio (which is annoying), the only fix is to implement createAnalyzer ? Anyway, happy to know why google music wasn't working correctly !

> the only fix is to implement createAnalyzer ? And that might well not be enough, depending on what else they're using.

I'll try to get to this soon.

Looks like Google Music is using getFloatFrequencyData from AnalyserNode. I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=21446 to get this node properly spec'ed.

Kiss FFT is a simple and cross platform FFT library which should be good enough for our FFT needs in Web Audio for now. This patch just imports the library and adds it to the build. Not a lot to review here... Note that Kiss FFT is also part of opus, but I'd rather have the library as a clear dependency which we can update later as opposed to hacking the libopus build system to let us use its Kiss FFT.

Comment on attachment 731753 [details] [diff] [review] Part 1: DOM bindings >+#include "AnalyserNode.h" "mozilla/dom/AnalyserNode.h", please. r=me, though you could make these attributes [Pure] if you wanted to, I think. Not sure whether it matters.

Comment on attachment 731753 [details] [diff] [review] Part 1: DOM bindings https://hg.mozilla.org/integration/mozilla-inbound/rev/81bad314826a With this patch, Google Music should in theory begin to work, just with no visualizations. I don't have an account yet so I can't test it myself...

Comment on attachment 731753 [details] [diff] [review] Part 1: DOM bindings Backed out because of build bustage: http://hg.mozilla.org/integration/mozilla-inbound/rev/d03af4bf5681

Comment on attachment 731753 [details] [diff] [review] Part 1: DOM bindings This is what I get for building patches on top of the ones that I have applied on my tree! Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/46e9d41acdeb

This incorporates some changes that I had to make to the build system to make things work.

This mimics what WebKit does to implement the AnalyserNode, pending upcoming spec clarifications. Testcase/demo: http://people.mozilla.com/~eakhgari/webaudio/analyser.html

Comment on attachment 732179 [details] [diff] [review] Part 2: Import Kiss FFT Review of attachment 732179 [details] [diff] [review]: ----------------------------------------------------------------- Hm, this is a build system patch, it should be reviewed by a build system person I guess. Bouncing off to glandium. Also, did you check that the licensing is okay? There should not be a problem considering this code is already present in Opus, and because Opus is already in the tree, but gerv could have a look. ::: media/kiss_fft/.hg_archival.txt @@ +1,4 @@ > +repo: d844c818f599aea64fe86745cdd2ef9b3d1910dc > +node: b354a59534b0a77c43c67deb1eb1bc39eb99b487 > +branch: default > +tag: v130 Are those .hg* files intended to be in the import? ::: media/kiss_fft/README.mozilla @@ +1,2 @@ > +This is the Kiss FFT library, version 1.3.0, retrieved from > +http://hivelocity.dl.sourceforge.net/project/kissfft/kissfft/v1_3_0/kiss_fft130.zip. Maybe you could document the process one should use to update the library? It seems to be a matter of simply doing `hg update`, but I'm not sure. Usually, we just have a script called `update.sh` that takes an upstream repo as $1, and pull in the new version, applying any patch that is needed in the process. Considering you Makefile.in references only one .c file, it could be cool to copy just the needed files. ::: media/kiss_fft/test/benchfftw.c @@ +32,5 @@ > + fprintf(stderr,"Datatype not available in FFTW\n" ); > + return 0; > +} > +#else > +int main(int argc,char ** argv) We probably don't want to compile that, and maybe we should not include it at all in the library version we have in-tree, that and all the files we don't use, such as benchmarks, python examples, matlab files, and other stuff. It might make the update process simpler, but the library is updated very rarely, so it should not matter.

Comment on attachment 732179 [details] [diff] [review] Part 2: Import Kiss FFT This is fine; but simply call it the "Kiss FFT License" in about:license. Gerv

Comment on attachment 732179 [details] [diff] [review] Part 2: Import Kiss FFT Review of attachment 732179 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, but... > "There should not be a problem considering this code is already present in Opus, and because Opus is already in the tree" So, why are we importing this code exactly?

BTW, a separation between the code import and the build bits would have been helpful for review.

glandium, Comment 25 says: > Note that Kiss FFT is also part of opus, but I'd rather have the library as a clear > dependency which we can update later as opposed to hacking the libopus build system to let > us use its Kiss FFT.

(In reply to Paul Adenot (:padenot) from comment #37) > glandium, Comment 25 says: > > > Note that Kiss FFT is also part of opus, but I'd rather have the library as a clear > > dependency which we can update later as opposed to hacking the libopus build system to let > > us use its Kiss FFT. Then can we make opus use this one? Because with two copies of the same thing, you can be sure that at some point, one will have fixes the other one won't.

Actually, it should be possible to use the opus kiss fft without modifying anything to the build system. Just include kiss_fft.h from $(topsrcdir)/media/libopus/celt, and prefix the function calls with opus_.

(In reply to Mike Hommey [:glandium] from comment #39) > Actually, it should be possible to use the opus kiss fft without modifying > anything to the build system. Just include kiss_fft.h from > $(topsrcdir)/media/libopus/celt, and prefix the function calls with opus_. The kiss_fft inside libopus lacks the tools directory, where kiss_fftr which I use in this code comes from. I don't really want to implement my own kiss_fftr on top of libopus' kiss_fft. Also, I'm not sure if the kiss_fft symbols in libopus are exported or not, since libopus lives in libgkmedias and the code using kiss_fftr lives in libxul.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #40) > (In reply to Mike Hommey [:glandium] from comment #39) > > Actually, it should be possible to use the opus kiss fft without modifying > > anything to the build system. Just include kiss_fft.h from > > $(topsrcdir)/media/libopus/celt, and prefix the function calls with opus_. > > The kiss_fft inside libopus lacks the tools directory, where kiss_fftr which > I use in this code comes from. I don't really want to implement my own > kiss_fftr on top of libopus' kiss_fft. Also, I'm not sure if the kiss_fft > symbols in libopus are exported or not, since libopus lives in libgkmedias > and the code using kiss_fftr lives in libxul. You could import kiss_fftr, make it use libopus's kiss_fft, and put it in libgkmedias.

(In reply to Paul Adenot (:padenot) from comment #33) > ::: media/kiss_fft/.hg_archival.txt > @@ +1,4 @@ > > +repo: d844c818f599aea64fe86745cdd2ef9b3d1910dc > > +node: b354a59534b0a77c43c67deb1eb1bc39eb99b487 > > +branch: default > > +tag: v130 > > Are those .hg* files intended to be in the import? I just imported the entire redistributable package. These files are not _needed_ and I can clean them up. > ::: media/kiss_fft/README.mozilla > @@ +1,2 @@ > > +This is the Kiss FFT library, version 1.3.0, retrieved from > > +http://hivelocity.dl.sourceforge.net/project/kissfft/kissfft/v1_3_0/kiss_fft130.zip. > > Maybe you could document the process one should use to update the library? > It seems to be a matter of simply doing `hg update`, but I'm not sure. Oh, I just downloaded the zip file, so the update process for future versions is to download the new zip archive and put its contents in this directory... > Usually, we just have a script called `update.sh` that takes an upstream > repo as $1, and pull in the new version, applying any patch that is needed > in the process. Considering you Makefile.in references only one .c file, it > could be cool to copy just the needed files. OK I'll add that. > ::: media/kiss_fft/test/benchfftw.c > @@ +32,5 @@ > > + fprintf(stderr,"Datatype not available in FFTW\n" ); > > + return 0; > > +} > > +#else > > +int main(int argc,char ** argv) > > We probably don't want to compile that, and maybe we should not include it > at all in the library version we have in-tree, that and all the files we > don't use, such as benchmarks, python examples, matlab files, and other > stuff. It might make the update process simpler, but the library is updated > very rarely, so it should not matter. Alright. These files were not built in my version of the patch, but I'll create another import which just copies the required files and nothing more.

(In reply to Mike Hommey [:glandium] from comment #41) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #40) > > (In reply to Mike Hommey [:glandium] from comment #39) > > > Actually, it should be possible to use the opus kiss fft without modifying > > > anything to the build system. Just include kiss_fft.h from > > > $(topsrcdir)/media/libopus/celt, and prefix the function calls with opus_. > > > > The kiss_fft inside libopus lacks the tools directory, where kiss_fftr which > > I use in this code comes from. I don't really want to implement my own > > kiss_fftr on top of libopus' kiss_fft. Also, I'm not sure if the kiss_fft > > symbols in libopus are exported or not, since libopus lives in libgkmedias > > and the code using kiss_fftr lives in libxul. > > You could import kiss_fftr, make it use libopus's kiss_fft, and put it in > libgkmedias. What would be the point of this exercise? You're assuming that these two libraries can be used interchangeably, that's not the case. The libopus version has diverged from upstream heavily and I'm not sure what the changes involved are. Also, it's compiled with -DFIXED_POINT=1 on mobile, but I do need the floating point version on mobile too since my input and output data are floats...

Comment on attachment 732182 [details] [diff] [review] Part 4: Implement the analysis routines of AnalyserNode Review of attachment 732182 [details] [diff] [review]: ----------------------------------------------------------------- This fails to build (with the four patches applied on an updated central tree). http://www.pastebin.mozilla.org/2267323 ::: content/media/webaudio/AnalyserNode.cpp @@ +188,5 @@ > + buffer[i] = static_cast<unsigned char>(scaled); > + } > +} > + > +bool You don't use the return value on the call site, maybe you could make this function void. @@ +196,5 @@ > + bool allocated = false; > + if (mWriteIndex == 0) { > + inputBuffer = mBuffer.Elements(); > + } else { > + inputBuffer = static_cast<float*>(moz_malloc(mFFTSize * sizeof(float))); Why |moz_malloc| instead of |new|? @@ +206,5 @@ > + allocated = true; > + } > + nsAutoArrayPtr<kiss_fft_cpx> outputBuffer(new kiss_fft_cpx[FrequencyBinCount() + 1]); > + > + ApplyBlackmanWindow(inputBuffer, mFFTSize); I could not find the window to use in the spec. Time to raise another spec issue, I believe. @@ +224,5 @@ > + (1.0 - mSmoothingTimeConstant) * scalarMagnitude; > + } > + > + if (allocated) { > + moz_free(inputBuffer); |delete []| instead of |moz_free| if you make the change above.

(In reply to Paul Adenot (:padenot) from comment #44) > Comment on attachment 732182 [details] [diff] [review] > Part 4: Implement the analysis routines of AnalyserNode > > Review of attachment 732182 [details] [diff] [review]: > ----------------------------------------------------------------- > > This fails to build (with the four patches applied on an updated central > tree). > > http://www.pastebin.mozilla.org/2267323 Yeah, sorry, I had a local patch applied which broke this. Please use the version 1 patch that I pushed to inbound yesterday and just apply parts 2-4. (They should apply and build fine on inbound.) > ::: content/media/webaudio/AnalyserNode.cpp > @@ +188,5 @@ > > + buffer[i] = static_cast<unsigned char>(scaled); > > + } > > +} > > + > > +bool > > You don't use the return value on the call site, maybe you could make this > function void. Oh that's my mistake, I'll fix it. This function can fail to allocate memory, hence the need for returning a bool. > @@ +196,5 @@ > > + bool allocated = false; > > + if (mWriteIndex == 0) { > > + inputBuffer = mBuffer.Elements(); > > + } else { > > + inputBuffer = static_cast<float*>(moz_malloc(mFFTSize * sizeof(float))); > > Why |moz_malloc| instead of |new|? To make the allocation fallible, since the allocation size comes from web content. operator new is infallible by default. If you want, I can use new(fallible) similar to here: <http://mxr.mozilla.org/mozilla-central/source/content/media/webaudio/MediaBufferDecoder.cpp#527> but I don't like that style for things which don't need to get constructed. > @@ +206,5 @@ > > + allocated = true; > > + } > > + nsAutoArrayPtr<kiss_fft_cpx> outputBuffer(new kiss_fft_cpx[FrequencyBinCount() + 1]); > > + > > + ApplyBlackmanWindow(inputBuffer, mFFTSize); > > I could not find the window to use in the spec. Time to raise another spec > issue, I believe. https://www.w3.org/Bugs/Public/show_bug.cgi?id=21446 covers that. :-) > @@ +224,5 @@ > > + (1.0 - mSmoothingTimeConstant) * scalarMagnitude; > > + } > > + > > + if (allocated) { > > + moz_free(inputBuffer); > > |delete []| instead of |moz_free| if you make the change above. Let me know if you want me to use operator new(fallible)...

Comment on attachment 732180 [details] [diff] [review] Part 3: Send the buffer seen by AnalyserNodeEngine to AnalyserNode Review of attachment 732180 [details] [diff] [review]: ----------------------------------------------------------------- I'll go and talk about what we should spec in the bug on the mailing list.

This version imports the minimum of Kiss FFT that we need into the tree, and it also adds an update.sh script for future updates.

This is just the build system changes. glandium said he wants to have another look at this.

Comment on attachment 732457 [details] [diff] [review] Part 3: Integrate Kiss FFT with the build system Review of attachment 732457 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/kiss_fft/Makefile.in @@ +9,5 @@ > + > +include $(DEPTH)/config/autoconf.mk > + > +LIBRARY_NAME = kiss_fft > +SHORT_LIBNAME = kiss_fft Don't set SHORT_LIBNAME. @@ +10,5 @@ > +include $(DEPTH)/config/autoconf.mk > + > +LIBRARY_NAME = kiss_fft > +SHORT_LIBNAME = kiss_fft > +VISIBILITY_FLAGS = You probably want to enclose this with ifeq (WINNT,$(OS_TARGET)) @@ +13,5 @@ > +SHORT_LIBNAME = kiss_fft > +VISIBILITY_FLAGS = > +EXPORTS_NAMESPACES = kiss_fft > + > +EXTRA_DSO_LDOPTS += $(MOZALLOC_LIB) Unnecessary. @@ +15,5 @@ > +EXPORTS_NAMESPACES = kiss_fft > + > +EXTRA_DSO_LDOPTS += $(MOZALLOC_LIB) > + > +EXPORTS_kiss_fft = kiss_fft.h kiss_fftr.h I assume you made the directory flat. (as opposed to the previous tools/ subdirectory)

(In reply to Mike Hommey [:glandium] from comment #51) > @@ +15,5 @@ > > +EXPORTS_NAMESPACES = kiss_fft > > + > > +EXTRA_DSO_LDOPTS += $(MOZALLOC_LIB) > > + > > +EXPORTS_kiss_fft = kiss_fft.h kiss_fftr.h > > I assume you made the directory flat. (as opposed to the previous tools/ > subdirectory) Yes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f7d9ca669a8 https://hg.mozilla.org/integration/mozilla-inbound/rev/93e66438fb71 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0c0b77f3022 https://hg.mozilla.org/integration/mozilla-inbound/rev/e5e56c43e359 Frederick, these patches fix Google Music for me. Would you mind retesting when this stuff hits Nightly? Thanks!

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #53) > https://hg.mozilla.org/integration/mozilla-inbound/rev/3f7d9ca669a8 > https://hg.mozilla.org/integration/mozilla-inbound/rev/93e66438fb71 > https://hg.mozilla.org/integration/mozilla-inbound/rev/a0c0b77f3022 > https://hg.mozilla.org/integration/mozilla-inbound/rev/e5e56c43e359 > > Frederick, these patches fix Google Music for me. Would you mind retesting > when this stuff hits Nightly? > > Thanks! Of course, I will test this ! Just waiting to see these patches to be integrated in main trunk code.

gcc cannot deal with template arguments that are local classes. Somehow that doesn't surprise me! Fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/581b88b27b2d

FWIW, we discussed the possibility of merging the kissfft used here with the copy in libopus, and given our constraints sharing them seems impractical at this point. Also it seems like the libopus copy has diverged heavily from upstream, so we might not be duplicating much code after all.

https://hg.mozilla.org/mozilla-central/rev/3f7d9ca669a8 https://hg.mozilla.org/mozilla-central/rev/93e66438fb71 https://hg.mozilla.org/mozilla-central/rev/a0c0b77f3022 https://hg.mozilla.org/mozilla-central/rev/e5e56c43e359 https://hg.mozilla.org/mozilla-central/rev/581b88b27b2d

Thanks to everybody. Glad to get webaudio enabled again and a working google music account !

Ehsan, do we need this on Aurora 22?

(In reply to Boris Zbarsky (:bz) from comment #60) > Ehsan, do we need this on Aurora 22? Hmm, I'm undecided on whether we should backport this or just disable Web Audio on Aurora 22... This fixes Google Music, but Google Music doesn't actually seem to use Web Audio for playback anyways, otherwise it would probably be broken on Aurora for other things we don't yet implement. On the other hand, other content which uses Web Audio should be broken to a good extent on Aurora 22 anyways... What do you think, Boris?

If we're planning to disable web audio on beta 22 no matter what (I assume we are?) then I'm not sure it's worth worrying about doing anything with this bug on Aurora...

(In reply to comment #62) > If we're planning to disable web audio on beta 22 no matter what (I assume we > are?) then I'm not sure it's worth worrying about doing anything with this bug > on Aurora... We're definitely not planning to enable Web Audio on Beta 22. The pref is currently enabled behind RELEASE_BUILD, which means Nightly and Aurora.

Given comment 63, we won't track or uplift this to 22.

Doc is up-to-date here: https://developer.mozilla.org/en-US/docs/Web/API/AnalyserNode