Bug 136127 – [EFL] Utilize espeak as a synthesizer back-end for WebSpeech

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Krzysztof Czech]

Utilize espeak as a synthesizer back-end for WebSpeech.

[Krzysztof Czech]

Created attachment 236925 [details]
patch

[Gyuyoung Kim]

I finished to install libespeak-dev to efl-ews and buildbot.

[Krzysztof Czech]

Created attachment 236971 [details]
attempt to check webkit-efl bot

[Gyuyoung Kim]

Comment on attachment 236971 [details]
attempt to check webkit-efl bot

View in context: https://bugs.webkit.org/attachment.cgi?id=236971&action=review

Any tests or unskip test by this patch ?

> Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:195
> +    };

ASSERT_NOT_REACHED() ?

[Krzysztof Czech]

(In reply to comment #4)
> (From update of attachment 236971 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236971&action=review
> 
> Any tests or unskip test by this patch ?
> 
> > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:195
> > +    };
> 
> ASSERT_NOT_REACHED() ?

Currently only Mac has supported speech synthesis and there're some tests that could be shared. There look generic

There are some tests in Mac, they seem generic and could be shared with other ports. I will add them in the next patch.

[Krzysztof Czech]

Created attachment 236984 [details]
patch

[Krzysztof Czech]

(In reply to comment #4)
> (From update of attachment 236971 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236971&action=review
> 
> Any tests or unskip test by this patch ?
> 
> > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:195
> > +    };
> 
> ASSERT_NOT_REACHED() ?
This object should never be deleted but just in case having this assert sounds fine with me.

DONE.

[Gyuyoung Kim]

Comment on attachment 236984 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236984&action=review

> Source/WebCore/platform/efl/PlatformSpeechSynthesizerEfl.cpp:50
> +    if (m_platformSpeechWrapper)

Any chance m_platformSpeechWrapper can be null though this is created ctor ?

Isn't ASSERT() better ?

> Source/cmake/OptionsEfl.cmake:96
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_SPEECH_SYNTHESIS ON)

If you want to add tests in next time, I think we should not enable this feature in this time.

> Tools/efl/install-dependencies:74
> +        libespeak-dev \

It would be good if you place this by alphabet order.

[Krzysztof Czech]

Created attachment 237073 [details]
patch

[Krzysztof Czech]

(In reply to comment #8)
> (From update of attachment 236984 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236984&action=review
> 
> > Source/WebCore/platform/efl/PlatformSpeechSynthesizerEfl.cpp:50
> > +    if (m_platformSpeechWrapper)
> 
> Any chance m_platformSpeechWrapper can be null though this is created ctor ?
Hmm, probably not
> 
> Isn't ASSERT() better ?
but I'm fine with having this ASSERT, done.
> 
> > Source/cmake/OptionsEfl.cmake:96
> > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_SPEECH_SYNTHESIS ON)
> 
> If you want to add tests in next time, I think we should not enable this feature in this time.
OK, done
> 
> > Tools/efl/install-dependencies:74
> > +        libespeak-dev \
> 
> It would be good if you place this by alphabet order.
Done

[Gyuyoung Kim]

Comment on attachment 237073 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237073&action=review

> Source/WebCore/platform/efl/PlatformSpeechSynthesizerEfl.cpp:51
> +    if (m_platformSpeechWrapper)

Why we still need to check if m_platformSpeechWrapper is null ? I meant m_platformSpeechWrapper can't be null. So, we don't need to check it. Thus, we can use ASSERT() because m_platformSpeechWrapper should not be null.

[Krzysztof Czech]

(In reply to comment #11)
> (From update of attachment 237073 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237073&action=review
> 
> > Source/WebCore/platform/efl/PlatformSpeechSynthesizerEfl.cpp:51
> > +    if (m_platformSpeechWrapper)
> 
> Why we still need to check if m_platformSpeechWrapper is null ? I meant m_platformSpeechWrapper can't be null. So, we don't need to check it. Thus, we can use ASSERT() because m_platformSpeechWrapper should not be null.

Yes, you're right, this can be deleted, sorry. I sometimes find myself too wary.

[Krzysztof Czech]

Created attachment 237075 [details]
patch

[Gyuyoung Kim]

Comment on attachment 237075 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237075&action=review

> Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:48
> +    ASSERT_NOT_REACHED();

Why here ?

> Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:78
> +            // Espeak adds an empty char at the beginning of the language

char -> character ?

> Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:93
> +    if (!m_isEngineStarted) {

How about using early return for better readability as below ?

    if (!m_isEngineStarted) {
        if (m_isEngineStarted = espeak_Initialize(AUDIO_OUTPUT_PLAYBACK, 0, 0, 0) == EE_INTERNAL_ERROR)
            return false;
   }
   espeak_SetVoiceByName("default");
  
   return true;

> Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:159
> +        fireSpeechEvent(SpeechError);

Don't you need to set m_utterance = nullptr; as well ?

[Krzysztof Czech]

(In reply to comment #14)
> (From update of attachment 237075 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237075&action=review
> 
> > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:48
> > +    ASSERT_NOT_REACHED();
> 
> Why here ?
I expect this object will never be deleted, but I'm not sure about it now.
> 
> > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:78
> > +            // Espeak adds an empty char at the beginning of the language
> 
> char -> character ?
Thanks
> 
> > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:93
> > +    if (!m_isEngineStarted) {
> 
> How about using early return for better readability as below ?
But then m_isEngineStarted points to EE_INTERNAL_ERROR that is OK, which is a bit not intuitive. Maybe something like this
if !(m_isEngineStarted = espeak_Initialize(AUDIO_OUTPUT_PLAYBACK, 0, 0, 0) != EE_INTERNAL_ERROR)
     return false;

Now when initialization is wrong, m_isEngineStarted is false.
But early return is fine, I will change the code.

> 
>     if (!m_isEngineStarted) {
>         if (m_isEngineStarted = espeak_Initialize(AUDIO_OUTPUT_PLAYBACK, 0, 0, 0) == EE_INTERNAL_ERROR)
>             return false;
>    }
>    espeak_SetVoiceByName("default");
> 
>    return true;
> 
> > Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:159
> > +        fireSpeechEvent(SpeechError);
> 
> Don't you need to set m_utterance = nullptr; as well ?
I agree.

[Krzysztof Czech]

Created attachment 237084 [details]
patch

[Build Bot]

Comment on attachment 237084 [details]
patch

Attachment 237084 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5637799936000000

New failing tests:
js/dom/line-column-numbers.html

[Build Bot]

Created attachment 237092 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Krzysztof Czech]

Created attachment 237136 [details]
attempt to check bots

[Krzysztof Czech]

Gyuyoung any chance to get final review ?

[Gyuyoung Kim]

Comment on attachment 237136 [details]
attempt to check bots

View in context: https://bugs.webkit.org/attachment.cgi?id=237136&action=review

Looks better than previous patch. Please land after fixing trivial nits.

> Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:35
>  

Unneeded line.

> Source/WebCore/platform/efl/PlatformSpeechSynthesisProviderEfl.cpp:120
> +    }

Need to add new line.

[Krzysztof Czech]

Committed r172956: <http://trac.webkit.org/changeset/172956>