[Mac 10.13]: Reader Mode TTS not working

VERIFIED FIXED in Firefox 57

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: winstonma, Assigned: m_kato)

Tracking

56 Branch
mozilla58
Unspecified
macOS
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 wontfix, firefox57 verified, firefox58 verified)

Details

Attachments

(1 attachment)

Reporter

Description

2 years ago
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
Keywords: regression

Comment 3

2 years ago
Eitan, any idea what's going on here?
Flags: needinfo?(eitan)

Updated

2 years ago
Blocks: highsierra
Component: Reader Mode → Web Speech
Product: Toolkit → Core

Comment 4

2 years ago
André, seems like you know about Web Speech, maybe you can help?
Flags: needinfo?(anatal)
Doesn't sound like a regression if this is new in 10.13.
Keywords: regression

Comment 6

2 years ago
I am more familiar with the recognition part. About TTS I think Eitan is the more appropriated contact.

Updated

2 years ago
Duplicate of this bug: 1405809

Updated

2 years ago
Flags: needinfo?(anatal)
Assignee

Comment 8

2 years ago
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)
Assignee

Comment 10

2 years ago
OK, patch is coming.
Assignee: nobody → m_kato
Flags: needinfo?(m_kato)
Comment hidden (mozreview-request)

Comment 13

2 years ago
mozreview-review
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+
Assignee

Comment 14

2 years ago
(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)

Comment 15

2 years ago
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/dd361e3ebf4d
Keep SpeechDelegate object until speaking is finished. r=eeejay
https://hg.mozilla.org/mozilla-central/rev/dd361e3ebf4d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Do we need to fix this in 57?  If so please request uplift when you get a chance.
Flags: needinfo?(m_kato)
Assignee

Comment 18

2 years ago
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.
Flags: needinfo?(m_kato)
Attachment #8917732 - Flags: approval-mozilla-beta?
Reporter

Comment 19

2 years ago
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!
Flags: needinfo?(tgrabowski)
Flags: needinfo?(sphilp)
Flags: needinfo?(m_kato)
Hi Jim, getting this on your radar too. Thanks!
Flags: needinfo?(jmathies)
Assignee

Comment 22

2 years ago
(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)
Assignee

Comment 23

2 years ago
Before landing this, TTS stops at 1st sentence.  So CPU consumption spends 1st sentence only.
Reporter

Comment 24

2 years ago
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%.
Flags: needinfo?(winstonma)
Assignee

Comment 25

2 years ago
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.
Flags: needinfo?(sphilp)
Reporter

Comment 27

2 years ago
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.

Updated

2 years ago
Flags: needinfo?(jmathies)
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?
Flags: needinfo?(rkothari)
I agree that it should be a follow up bug. Thanks for your due diligence Stuart. I'll take this patch in 57.
Flags: needinfo?(rkothari)
Assignee

Comment 30

2 years ago
(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.
Reporter

Comment 31

2 years ago
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+
Flags: needinfo?(tgrabowski)
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.