1032111 - [RTSP] Basic RTSP test case on ICS emulator

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RTSP, defect)

Description

[Ethan Tseng [:ethan]]

This bug aims to establish the basic test case for RTSP feature.

Comment 1

[Ethan Tseng [:ethan]]

Related bug: Bug 1003711 - [RTSP] Build Darwin Streaming Server on B2G emulator

Comment 2

[Ethan Tseng [:ethan]]

Attachment: bug-1032111-wip.patch

A WIP patch that only starts up DSS.

Comment 3

[Ethan Tseng [:ethan]]

Attachment: bug-1032111-wip.patch

A WIP patch.

Comment 4

[Ethan Tseng [:ethan]]

Attachment: bug-1032111-wip.patch

A WIP patch.

Comment 5

[Ethan Tseng [:ethan]]

Hi Steve,

I am trying to write RTSP unit test on marionette framework, and we've got a problem.
Marionette can be executed only in chrome process, not content (It's limited by several reasons, however, that's the fact for now).
I guess you remember that our RtspChannel is only implemented in the child side, while the RtspChannelParent exists just for HTTP->RTSP redirection and nothing else. This puts a limitation to our RTSP function that it can only be initialized from the content process. Marionette test cannot load it.
I think HttpChannel doesn't have this limitation. It can be loaded in both content and chrome process.

When I was refactoring RtspChannel in bug 992568, I was not aware of this OOP issue.
It seems we should refactor it again to make it work in chrome process (OOP = false).

I'd like to know what you think.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Hi Ethan,

Are you sure marionette can't be run in content? MDN says that the context defaults to content - https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Marionette_JavaScript_Tests see MARIONETTE_CONTEXT.

If this page is not updated, then I think you might consider writing some stub functions to send messages from chrome to content. I don't think we want to start refactoring things back to the chrome process just for tests. I'm not excited about stub functions either :) but I'd rather have them than a big refactoring.

Comment 7

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #6)
> Hi Ethan,
> 
> Are you sure marionette can't be run in content? MDN says that the context
> defaults to content -
> https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/
> Marionette_JavaScript_Tests see MARIONETTE_CONTEXT.

Hi Steve, thanks for you suggestion.

After reading your comment and rethinking the problem, I found I should not use the terms
"chrome vs content process", which are confusing here.
Instead, the problem should be "parent vs child process".
We developed and tested RTSP on B2G only, which enables oop (out-of-process) mode.

But when we run Marionette test, oop mode is off (oop = false).
RtspChannel will be created by a parent process instead of a child process but we don't support that.

                  IsNeckoChild()     XRE_GetProcessType()
Browser App       true (1)           GeckoProcessType_Content (2)
Marionette Test   false (0)          GeckoProcessType_Default (0)

Do you think we should fix this to make RTSP work without oop mode?

p.s. You are right. Marionette test codes can be run in content process as its context.

Comment 8

[Ethan Tseng [:ethan]]

(In reply to Ethan Tseng [:ethan] from comment #7)
> But when we run Marionette test, oop mode is off (oop = false).
BTW, bug 970804 added support to marionette tests running in oop mode.
I will also try to utilize this support as a workaround for tests.
Not sure it's feasible or not.
(I tried to set "oop = true" in my marionette test but the test script failed to run).

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

Removing needinfo as we talked about this offline.

Comment 10

[Ethan Tseng [:ethan]]

(In reply to Ethan Tseng [:ethan] from comment #8)
> BTW, bug 970804 added support to marionette tests running in oop mode.
> I will also try to utilize this support as a workaround for tests.
> Not sure it's feasible or not.
> (I tried to set "oop = true" in my marionette test but the test script
> failed to run).

Bug 1007062 - SpecialPowers doesn't work in Marionette OOP mode
Bug 1058158 - Marionette WebAPI tests should run in either a test container or a clean environment

These two bugs were fixed and realized the support of SpecialPowers in marionette OOP mode.
Now we can use "test_container = true" to enable OOP mode in marionette test.
Basically, RTSP stream can be loaded under this situation.
More follow-ups are needed to complete the test case.

Comment 11

[Ethan Tseng [:ethan]]

Attachment: emulator-5554.log

Unexpected error happens while loading an RTSP stream in marionette test.

Comment 12

[Ethan Tseng [:ethan]]

Hi Blake and Bruce,

I am trying to execute a marionette test that loads RTSP stream and the test case will be passed when
it receives the ended event.
However, an unexpected error always happens in the middle of stream.
Do you have any idea or clue on how to debug this issue?

The media is H.264 baseline profile.
I/OMXCodec(  287): [OMX.google.h264.decoder] AVC profile = 66 (Baseline), level = 30
I/OMXCodec(  287): [OMX.google.h264.decoder] video dimensions are 320 x 240
I/OMXCodec(  287): [OMX.google.h264.decoder] Crop rect is 320 x 240 @ (0, 0)

The logcat is attached.
It seems no much useful information in the logcat output.
I plan to make a debug build to generate more logs.

Comment 13

[Ethan Tseng [:ethan]]

Attachment: bug-1032111-wip.patch

Add addEventListener() to wait for the ended event.

Comment 14

[Ethan Tseng [:ethan]]

BTW, I used the same test case to load an HTTP stream media (also H.264 with baseline profile).
It can successfully get the ended event and the test case passed.

I guess both RTSP and HTTP stream in marionette test are processed by the same software decoder.
The unexpected error (crash) might indicate an exception in our RTSP stack. I'll figure it out.

Do you have experience to play H.264 media in marionette test?

Comment 15

[Ethan Tseng [:ethan]]

Bug 1086198 is found using the WIP patch in this bug.

Comment 16

[Ethan Tseng [:ethan]]

The JS test script cannot receive "ended" media event.
Sometimes it even crashes when the playback reaches the end of media.
Bug 1088538 was filed to track the crash issue.

Comment 17

[Blake Wu [:bwu][:blakewu]]

Per discussion offline, it looks currently video can be played. Good to know!

Comment 18

[Ethan Tseng [:ethan]]

(In reply to Ethan Tseng [:ethan] from comment #12)
> I am trying to execute a marionette test that loads RTSP stream and the test
> case will be passed when
> it receives the ended event.
> However, an unexpected error always happens in the middle of stream.

We found that the occasional crash or timeout are caused by our RTSP code.
We are working on these issues in other bugs.
The only problem with marionette is that, the video frames cannot be rendered on the screen.
For automation test purpose, I'll focus on verifying media events and ignore rendering issue for now.

Comment 19

[Ethan Tseng [:ethan]]

Attachment: bug-1032111-wip.patch

The draft RTSP test script.
It contains only one basic test case for video H.264.

Henry,
Could you provide some feedback for this patch?
Should we wrapDomRequestAsPromise() for any DOM manipulations?
I didn't wrap document.createElement() in this patch.

Comment 20

[Ethan Tseng [:ethan]]

Comment on attachment 8514166 [details] [diff] [review]
bug-1032111-wip.patch

As Hsin-Yi mentioned offline, bug 839838 implemented DOMRequest.then().
We don't need to wrap DOMRequest as promise by ourselves.
Reference:
http://hg.mozilla.org/mozilla-central/rev/a14b8bf72b16

So I cancel the feedback request.
I'll update my patch according to this information.

Comment 21

[Ethan Tseng [:ethan]]

Attachment: bug-1032111-wip.patch

Refine the patch. Remove codes to wrap DOMRequest.

Hi Henry,
Could you provide some feedback? Thanks!

Comment 22

[Ethan Tseng [:ethan]]

*** Memorandum ***

(In reply to Ethan Tseng [:ethan] from comment #10)
> Bug 1007062 - SpecialPowers doesn't work in Marionette OOP mode
> Bug 1058158 - Marionette WebAPI tests should run in either a test container
> or a clean environment
> These two bugs were fixed and realized the support of SpecialPowers in
> marionette OOP mode.
> Now we can use "test_container = true" to enable OOP mode in marionette test.

Bug 1088538 -  [RTSP] System crash in the end of playback due to NULL accessUnit
This bug fixed the crash issue after end-of-stream.
So RTSP test on marionette can receive the "ended" media event now.

Comment 23

[Henry Chang [:hchang]]

Comment on attachment 8514854 [details] [diff] [review]
bug-1032111-wip.patch

Review of attachment 8514854 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Ethan! Looks good in general. Some comments are left for discussion.

::: netwerk/protocol/rtsp/test/marionette/test_rtsp_video_h264.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +MARIONETTE_TIMEOUT = 300000;  // 300 sec

I am not sure if this timeout is too long ....

@@ +9,5 @@
> +
> +gTestSuite.doTest(function () {
> +  return Promise.resolve()
> +    .then(() => gTestSuite.ensureWifiEnabled(false))
> +    .then(() => gTestSuite.requestWifiEnabled(true))

Why not just 

.then(() => gTestSuite.ensureWifiEnabled(true))

?

@@ +10,5 @@
> +gTestSuite.doTest(function () {
> +  return Promise.resolve()
> +    .then(() => gTestSuite.ensureWifiEnabled(false))
> +    .then(() => gTestSuite.requestWifiEnabled(true))
> +    .then(() => gTestSuite.startDarwinStreamingServer())

Whenever there're other test cases, we can move startDarwinStreamingServer/stopDarwinStreamingServer
into doTest function. But it's okay at this time since there's only one test case.

@@ +13,5 @@
> +    .then(() => gTestSuite.requestWifiEnabled(true))
> +    .then(() => gTestSuite.startDarwinStreamingServer())
> +    .then(() => gTestSuite.loadVideoElement(URL))
> +    .then(() => gTestSuite.waitForMediaEventOnce('loadeddata'))
> +    .then(() => gTestSuite.waitForMediaEventOnce('ended'))

The event is likely to be fired before we begin to wait. We should probably push all these promises to an array and use "Promise.all()" to prevent from the racing.

Comment 24

[Ethan Tseng [:ethan]]

(In reply to Henry Chang [:henry] from comment #23)
> ::: netwerk/protocol/rtsp/test/marionette/test_rtsp_video_h264.js
> @@ +1,5 @@
> > +MARIONETTE_TIMEOUT = 300000;  // 300 sec
> I am not sure if this timeout is too long ....

The video clip being tested is 70-sec long.
In real tests it takes about 72~73 seconds to complete the media playback.
Setting the timeout as 120 sec should be quite safe enough.


> @@ +9,5 @@
> > +gTestSuite.doTest(function () {
> > +  return Promise.resolve()
> > +    .then(() => gTestSuite.ensureWifiEnabled(false))
> > +    .then(() => gTestSuite.requestWifiEnabled(true))
> Why not just 
> .then(() => gTestSuite.ensureWifiEnabled(true))

You are right!
I'll change this.


> @@ +10,5 @@
> > +gTestSuite.doTest(function () {
> > +  return Promise.resolve()
> > +    .then(() => gTestSuite.ensureWifiEnabled(false))
> > +    .then(() => gTestSuite.requestWifiEnabled(true))
> > +    .then(() => gTestSuite.startDarwinStreamingServer())
> Whenever there're other test cases, we can move
> startDarwinStreamingServer/stopDarwinStreamingServer
> into doTest function. But it's okay at this time since there's only one test
> case.

Hmm... 
Actually both Wifi and streaming server are prerequisites for RTSP tests.
I'll move both of them into the doTest function.
Then all test cases don't have to bother with them.

 
> @@ +13,5 @@
> > +    .then(() => gTestSuite.requestWifiEnabled(true))
> > +    .then(() => gTestSuite.startDarwinStreamingServer())
> > +    .then(() => gTestSuite.loadVideoElement(URL))
> > +    .then(() => gTestSuite.waitForMediaEventOnce('loadeddata'))
> > +    .then(() => gTestSuite.waitForMediaEventOnce('ended'))
> The event is likely to be fired before we begin to wait. We should probably
> push all these promises to an array and use "Promise.all()" to prevent from
> the racing.

I didn't think of this!
Thanks for your advice. I'll try it!

Comment 25

[Ethan Tseng [:ethan]]

Attachment: bug-1032111.patch

Refinement according to Henry's suggestions.

Comment 26

[Ethan Tseng [:ethan]]

Comment on attachment 8517289 [details] [diff] [review]
bug-1032111.patch

Henry and Steve,

Could you help to review the patch?

Comment 27

[Ethan Tseng [:ethan]]

Attachment: bug-1032111.patch

waitForMediaEventOnce(), which calls addEventListener(), should be called before
the start of the playback. So we can prevent racing problem addressed in
comment 23.

Comment 28

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8517361 [details] [diff] [review]
bug-1032111.patch

Review of attachment 8517361 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good as a starting point for more test cases. r=me.

::: netwerk/protocol/rtsp/test/marionette/test_rtsp_video_h264.js
@@ +12,5 @@
> +  return Promise.all([
> +      gTestSuite.createVideoElement(URL),
> +      gTestSuite.waitForMediaEventOnce('loadeddata'),
> +      gTestSuite.waitForMediaEventOnce('ended'),
> +      gTestSuite.playVideoElement()

I'm not 100% familiar with Promises, but it looks like only waitForMediaEventOnce returns a promise in the list here. createVideoElement returns true, and playVideoElement doesn't have a return type. Is this ok?

Also, did you want to add pause here too?

Comment 29

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #28)
> I'm not 100% familiar with Promises, but it looks like only
> waitForMediaEventOnce returns a promise in the list here. createVideoElement
> returns true, and playVideoElement doesn't have a return type. Is this ok?

I make createVideoElement() always return true because I assume its operations always succeed.
Henry, what do you think?
Do we have to return "Promise.resolve()" instead?

Comment 30

[Henry Chang [:hchang]]

(In reply to Ethan Tseng [:ethan] from comment #29)
> (In reply to Steve Workman [:sworkman] from comment #28)
> > I'm not 100% familiar with Promises, but it looks like only
> > waitForMediaEventOnce returns a promise in the list here. createVideoElement
> > returns true, and playVideoElement doesn't have a return type. Is this ok?
> 
> I make createVideoElement() always return true because I assume its
> operations always succeed.
> Henry, what do you think?
> Do we have to return "Promise.resolve()" instead?

Returning |true| is good enough. However, you have to put gTestSuite.playVideoElement()
outside of Promise.all() such as:

return Promise.all([
  gTestSuite.waitForMediaEventOnce('loadeddata'),
  gTestSuite.waitForMediaEventOnce('ended'),]
  gTestSuite.createVideoElement(URL),
.then(gTestSuite.playVideoElement());

Promise.all() itself would also return a promise which
get resolved whenever all the associated promises have 
been resolved. 

Besides, you will have to call waitForMediaEventOnce()
before the event gets fired. Otherwise, you might miss
the event.

Comment 31

[Ethan Tseng [:ethan]]

Attachment: bug-1032111.patch

Refine the patch according to review's comments.

Comment 32

[Ethan Tseng [:ethan]]

Henry,

I just made two changes in the latest patch.
1. Return values of createVideoElement() and playVideoElement()
   Instead of returning |true|, make them return |Promise.resolve()|, which is more clear and is 
   "thenable".
2. Elements in Promise.all()
   createVideoElement(), waitForMediaEventOnce('loadeddata') and playVideoElement() are wrapped in
   Promise.all().
   When they are all resolved, then we do waitForMediaEventOnce('ended').
   I think this is more clear and should eliminate the racing issue.

What do you think?

Comment 33

[Ethan Tseng [:ethan]]

Attachment: bug-1032111.patch

As discussed offline, moving the waitForEvent function into the array of
Promise.all() is still a safer way.

Comment 34

[Ethan Tseng [:ethan]]

Attachment: RTSP automation test

Add RTSP test case into marionette-webapi set.

Comment 35

[Ethan Tseng [:ethan]]

I wanna try the test case, but it seems Treeherder doesn't perform any marionette-webapi test.
The syntax:
try: -b do -p all -u marionette-webapi -t none
The result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=036464f24afb

Ryan, can you help to take a look and see what's wrong with it?

Comment 36

[Ethan Tseng [:ethan]]

I noticed the "Mnw" would flash for a short period of time in the row "B2G ICS Emulator opt".
But it soon disappears.

Comment 37

[Ethan Tseng [:ethan]]

Oh, we can see the Mnw busted information from TBPL:
https://tbpl.mozilla.org/?tree=Try&rev=036464f24afb

Comment 38

[Ethan Tseng [:ethan]]

The Try result reports ScriptTimeoutException in Wifi test case (test_wifi_tethering_enabled.js).
So I temporarily removed test cases of Wifi and tethering.
Then the same timeout exception raises for another script (test_cellbroadcast_umts.js).
https://tbpl.mozilla.org/?tree=Try&rev=49a2dbdb4239

I think something is wrong in my test script. Need to find it out.
Henry and Chuck, could you provide some suggestion?

Comment 39

[Henry Chang [:hchang]]

According the following log, it might be due to the failure of running 
a test case in test container... test_cellbroadcast_umts.js was passed
in fact.

03:03:13     INFO -  Traceback (most recent call last):
03:03:13     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/runtests.py", line 40, in <module>
03:03:13     INFO -      cli()
03:03:13     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/runtests.py", line 35, in cli
03:03:13     INFO -      runner = startTestRunner(runner_class, options, tests)
03:03:13     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/runtests.py", line 20, in startTestRunner
03:03:13     INFO -      runner.run_tests(tests)
03:03:13     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/runner/base.py", line 742, in run_tests
03:03:13     INFO -      self.run_test_sets()
03:03:13     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/runner/base.py", line 926, in run_test_sets
03:03:13     INFO -      self.run_test_set(self.tests)
03:03:13     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/runner/base.py", line 907, in run_test_set
03:03:13     INFO -      self.run_test(test['filepath'], test['expected'], test['test_container'])
03:03:13     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/runner/base.py", line 881, in run_test
03:03:13     INFO -      self.launch_test_container()
03:03:13     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/runner/base.py", line 687, in launch_test_container
03:03:13     INFO -      }""", script_timeout=60000)
03:03:13     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/marionette.py", line 1295, in execute_async_script
03:03:13     INFO -      filename=os.path.basename(frame[0]))
03:03:13     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/decorators.py", line 36, in _
03:03:13     INFO -      return func(*args, **kwargs)
03:03:13     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/marionette.py", line 643, in _send_message
03:03:13     INFO -      self._handle_error(response)
03:03:13     INFO -    File "/builds/slave/test/build/tests/marionette/marionette/marionette.py", line 705, in _handle_error
03:03:13     INFO -      raise errors.ScriptTimeoutException(message=message, status=status, stacktrace=stacktrace)
03:03:13     INFO -  errors.ScriptTimeoutException: ScriptTimeoutException: timed out
03:03:13    ERROR - Return code: 1

Comment 40

[Henry Chang [:hchang]]

Comment on attachment 8519845 [details] [diff] [review]
RTSP automation test

Review of attachment 8519845 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 41

[Ethan Tseng [:ethan]]

Discussed offline with Edgar and Henry.
RTSP test script must run in OOP mode (test_container = true).
However, once marionette test has performed in-process mode test, it can no longer perform tests
in OOP mode.
Bug 1075437 is aimed to fix this problem by re-ordering test scripts to run OOP mode tests first.

Edgar, correct me if I misunderstand anything.

Comment 42

[Jonathan Hao (inactive) [:jhao]]

I try to run this test on my PC, and it always timeout.
I put some log in HTMLMediaElement::Play() and it turns out that its mDecoder is NULL.

Comment 43

[Jonathan Hao (inactive) [:jhao]]

It turns out that my command was wrong. I used 
./mach marionette-webapi mozilla-central/netwerk/protocol/rtsp/test/marionette/test_rtsp_video_h264.js

That doesn't enable OOP mode, the correct command is
./mach marionette-webapi mozilla-central/netwerk/protocol/rtsp/test/marionette/manifest.ini

Comment 44

[Ethan Tseng [:ethan]]

No one is actually working on RTSP for FxOS right now.