Closed Bug 1406358 Opened 4 years ago Closed 4 years ago
.13]: Reader Mode TTS not working
59 bytes, text/x-review-board-request
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36 Steps to reproduce: Make sure the Mac is running macOS 10.13 (I am not sure about older version but I am using macOS 10.13) and Firefox 56 installed. 1. Run Firefox without any addon (or Safe mode) 2. Go to http://mentalfloss.com/article/504666/bit-bit-inside-rise-retro-gaming 3. After the web page finished loading, enter Reader View 4. After the Reader View finished loading, press the read button of the left and then press the play button Actual results: On my Mac it only reads the website domain 'mentalfloss.com' then TTS stops. Expected results: It should be able to read the whole article
Component: Untriaged → Reader Mode
OS: Unspecified → Mac OS X
Product: Firefox → Toolkit
I can confirm this doesn't work on 56, 57 beta or 58 on my 10.13 machine. I can confirm that it works on my 10.12 machine on 57 beta. So appears to be a 10.13 only issue.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Adding tracking flags.
Eitan, any idea what's going on here?
Component: Reader Mode → Web Speech
Product: Toolkit → Core
André, seems like you know about Web Speech, maybe you can help?
Doesn't sound like a regression if this is new in 10.13.
I am more familiar with the recognition part. About TTS I think Eitan is the more appropriated contact.
We shouldn't call [delegate release ] after setDelegate.
Makoto, can you take this bug? Looks like you know the fix.
Flags: needinfo?(eitan) → needinfo?(m_kato)
OK, patch is coming.
Assignee: nobody → m_kato
Comment on attachment 8917732 [details] Bug 1406358 - Keep SpeechDelegate object until speaking is finished. https://reviewboard.mozilla.org/r/188664/#review194150 The ownership here is a bit confusing.. from what I understand: 1. nsSpeechTask holds a strong reference to SpeechTaskCallback (its actually a cyclical reference) 2. SpeechTaskCallback holds a strong reference to the SpeechDelegate (it creates it, and then releases its single reference in the destructor) 3. The SpeechDelegate holds a weak reference to the SpeechTaskCallBack. Did I get that right? Maybe a comment about (2) when you create SpeechDelegate because the mix of c++/objc reference counting is confusing, and it does not look like an owning reference.
Attachment #8917732 - Flags: review?(eitan) → review+
(In reply to Eitan Isaacson [:eeejay] from comment #13) > Comment on attachment 8917732 [details] > Bug 1406358 - Keep SpeechDelegate object until speaking is finished. > > https://reviewboard.mozilla.org/r/188664/#review194150 > > The ownership here is a bit confusing.. from what I understand: > 1. nsSpeechTask holds a strong reference to SpeechTaskCallback (its actually > a cyclical reference) > 2. SpeechTaskCallback holds a strong reference to the SpeechDelegate (it > creates it, and then releases its single reference in the destructor) > 3. The SpeechDelegate holds a weak reference to the SpeechTaskCallBack. > > Did I get that right? Maybe a comment about (2) when you create > SpeechDelegate because the mix of c++/objc reference counting is confusing, > and it does not look like an owning reference. 10.12 is NSSpeechSynthesizer has strong reference of SpeechDelegate. So when NSSpeechSynthesizer is destroyed in destructor, SpeechDelegate is released and destroyed. So new code will be same. (destructor destroys both NSSpeechSynthesizer and SpeechDelegate)
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/dd361e3ebf4d Keep SpeechDelegate object until speaking is finished. r=eeejay
Do we need to fix this in 57? If so please request uplift when you get a chance.
Comment on attachment 8917732 [details] Bug 1406358 - Keep SpeechDelegate object until speaking is finished. Approval Request Comment [Feature/Bug causing the regression]: Upgrade to macOS 10.13 [User impact if declined]: Web Speech API only speaks first sentence only. [Is this code covered by automated tests?]: No. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: Yes, See https://bugzilla.mozilla.org/show_bug.cgi?id=1406358#c0 [List of other uplifts needed for the feature/fix]: N/A [Is the change risky?]: Middle. [Why is the change risky/not risky?]: I change reference counting for objective-c object. It might be memory leak. [String changes made/needed]: No.
Attachment #8917732 - Flags: approval-mozilla-beta?
I tried it on Firefox Nightly and it is working. But the CPU consumption is around 50% on my MBP (the windows version is running at 5%).
Hello guys, the previous comment about the CPU usage on MacBook Pro as compared to the windows OS is worrisome. I'd like to see if this is an easily reproducible issue and related to the patch in this bug or unrelated before making a decision on the beta uplift. Any help is appreciated. Thanks!
Hi Jim, getting this on your radar too. Thanks!
(In reply to Winston from comment #19) > I tried it on Firefox Nightly and it is working. But the CPU consumption is > around 50% on my MBP (the windows version is running at 5%). What is macOS version? 10.13? and is reproduce step comment #0?
Flags: needinfo?(m_kato) → needinfo?(winstonma)
Before landing this, TTS stops at 1st sentence. So CPU consumption spends 1st sentence only.
Makoto, I ran the Firefox Nightly on macOS 10.13. Yes the test case still valid. The only additional thing is opening the activity monitor (task manager). You open the article in Reader Mode, open the activity monitor, wait until the CPU load is idle and then press the play button. You will find that the loading will jump to 50%.
https://perfht.ml/2zPSwG1 Not TTS issue. Speech doesn't show on profiler. But most is painting on content (Narreta will update content for maker everytime). So comment #19 doesn't depends on this bug.
Just tried this now on 58.0a1 (2017-10-29), OSX 10.13. ~15-35% CPU usage (I'm on a 2017 mbp, may be faster generally and use less CPU. You also want to be careful as a few browser components have a delayed start for the first 60 seconds or so after browser start up, so CPU usage can seemingly spike randomly at first (that's not what's going on here, but just to note, wait at least 60 seconds before measuring). I ran this as well on 52esr for comparison and get 15-25% CPU usage there, even though the functionality is broken (same as this original bug, it narrates the first line only, then continues to look as if it is narrating, but the actual voice output stops). imo this is overall an improvement. Seeing as this functionality has been broken since at least 52, and this patch now has it working, that's worth while itself. If there is a CPU regression, it seems to be < 10% on my machine (hard to say without more in depth profiling exactly). Whether it is acceptable and worth taking an uplift for, that might be a better call for product, but imo it's fine to take. QA should add a regression case for this if we haven't already.
Hi Stuart, I tried this and that really improve the CPU load. 1. https://developer.microsoft.com/en-us/microsoft-edge/testdrive/demos/speechsynthesis/ 2. Copy the text inside the textbox 3. Press the play button The CPU load will goes from 50% to 10% (it differs from laptop to laptop, but the improvement is significant). So it makes me think that the problem is inside Reader Mode. But have no idea which part is it.
That's awesome, thanks Winston. What might be appropriate here is we file a follow up to investigate what in reader view is eating up the CPU (since this bug is about functionality), what do you think Ritu?
I agree that it should be a follow up bug. Thanks for your due diligence Stuart. I'll take this patch in 57.
(In reply to Stuart Philp :sphilp from comment #28) > That's awesome, thanks Winston. What might be appropriate here is we file a > follow up to investigate what in reader view is eating up the CPU (since > this bug is about functionality), what do you think Ritu? Winston filed as Bug 1406360.
Makoto, thanks for finding the root cause of the high CPU load. I think it is better to have your findings (perf.html) filed in bug 1406360. Thanks
Comment on attachment 8917732 [details] Bug 1406358 - Keep SpeechDelegate object until speaking is finished. Recent regression, Beta57+
Attachment #8917732 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I've reproduced the initial issue (steps from Description) on macOS 10.13.1 using Firefox 56.0. I've tested on macOS 10.13.1 using Firefox 57 Beta 14 (buildID: 20171102181127) and latest Nightly 58.0a1 (2017-11-03) and Reader Mode TTS is correctly working, all the text is read.
You need to log in before you can comment on or make changes to this bug.