Bug 136127 - [EFL] Utilize espeak as a synthesizer back-end for WebSpeech
Summary: [EFL] Utilize espeak as a synthesizer back-end for WebSpeech
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Krzysztof Czech
URL:
Keywords:
Depends on:
Blocks: 116314
  Show dependency treegraph
 
Reported: 2014-08-21 05:32 PDT by Krzysztof Czech
Modified: 2014-08-26 04:26 PDT (History)
11 users (show)

See Also:


Attachments
patch (15.68 KB, patch)
2014-08-21 05:44 PDT, Krzysztof Czech
no flags Details | Formatted Diff | Diff
attempt to check webkit-efl bot (15.68 KB, patch)
2014-08-22 00:22 PDT, Krzysztof Czech
no flags Details | Formatted Diff | Diff
patch (15.69 KB, patch)
2014-08-22 07:17 PDT, Krzysztof Czech
no flags Details | Formatted Diff | Diff
patch (15.43 KB, patch)
2014-08-25 03:47 PDT, Krzysztof Czech
no flags Details | Formatted Diff | Diff
patch (14.97 KB, patch)
2014-08-25 04:23 PDT, Krzysztof Czech
no flags Details | Formatted Diff | Diff
patch (15.02 KB, patch)
2014-08-25 08:08 PDT, Krzysztof Czech
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (490.72 KB, application/zip)
2014-08-25 10:25 PDT, Build Bot
no flags Details
attempt to check bots (15.02 KB, patch)
2014-08-26 00:27 PDT, Krzysztof Czech
gyuyoung.kim: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Czech 2014-08-21 05:32:24 PDT
Utilize espeak as a synthesizer back-end for WebSpeech.
Comment 1 Krzysztof Czech 2014-08-21 05:44:32 PDT
Created attachment 236925 [details]
patch
Comment 2 Gyuyoung Kim 2014-08-21 17:52:37 PDT
I finished to install libespeak-dev to efl-ews and buildbot.
Comment 3 Krzysztof Czech 2014-08-22 00:22:11 PDT
Created attachment 236971 [details]
attempt to check webkit-efl bot
Comment 4 Gyuyoung Kim 2014-08-22 00:36:30 PDT
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() ?
Comment 5 Krzysztof Czech 2014-08-22 02:12:39 PDT
(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.
Comment 6 Krzysztof Czech 2014-08-22 07:17:34 PDT
Created attachment 236984 [details]
patch
Comment 7 Krzysztof Czech 2014-08-22 07:18:40 PDT
(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.
Comment 8 Gyuyoung Kim 2014-08-22 21:01:48 PDT
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.
Comment 9 Krzysztof Czech 2014-08-25 03:47:22 PDT
Created attachment 237073 [details]
patch
Comment 10 Krzysztof Czech 2014-08-25 03:49:53 PDT
(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
Comment 11 Gyuyoung Kim 2014-08-25 03:55:00 PDT
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.
Comment 12 Krzysztof Czech 2014-08-25 04:12:49 PDT
(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.
Comment 13 Krzysztof Czech 2014-08-25 04:23:51 PDT
Created attachment 237075 [details]
patch
Comment 14 Gyuyoung Kim 2014-08-25 06:14:50 PDT
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 ?
Comment 15 Krzysztof Czech 2014-08-25 07:34:34 PDT
(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.
Comment 16 Krzysztof Czech 2014-08-25 08:08:17 PDT
Created attachment 237084 [details]
patch
Comment 17 Build Bot 2014-08-25 10:24:55 PDT
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
Comment 18 Build Bot 2014-08-25 10:25:01 PDT
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
Comment 19 Krzysztof Czech 2014-08-26 00:27:19 PDT
Created attachment 237136 [details]
attempt to check bots
Comment 20 Krzysztof Czech 2014-08-26 02:24:37 PDT
Gyuyoung any chance to get final review ?
Comment 21 Gyuyoung Kim 2014-08-26 03:12:10 PDT
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.
Comment 22 Krzysztof Czech 2014-08-26 04:26:39 PDT
Committed r172956: <http://trac.webkit.org/changeset/172956>