1002804 - GTest for TaskQueue

Status: NEW

(Core :: Audio/Video: Playback, defect, P5)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: MediaTaskQueue-cpp-test.patch

I wrote a C++ unit test for MediaTaskQueue, but couldn't get it to link or run properly on Windows. It should really be a gtest anyway.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

What was the problem on Windows?

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

I was able to get the test to run locally by calling the EXE directly, but I couldn't get it to run as part of the test suite. IIRC I also had trouble writing a test for SharedThreadPool because we don't link libxul into the test EXEs directly, so you can't access non exported symbols, and SharedThreadPool at the time required calling out to the pref service. You can statically link into GTest (I think), but GTest doesn't work on Windows, so I gave up.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

It looks like |./mach gtest| doesn't work on Windows.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yeah :(

Bug 883339.

Comment 5

[Nicolas Vignes]

Try to compile your patch on OS X, but I got few errors. 

Seems you are trying to create a MediaTaskQueue with a nsCOMPtr<nsIThreadPool> instead of TemporaryRef<mozilla::SharedThreadPool>

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

The patch needs fixing and the test it adds should be converted to a gtest.

Comment 7

[Nicolas Vignes]

Attachment: bug-1002804-tests.patch

This is the Gtest version of the first patch. It compiles and executes correctly but we cannot use all the function like |Dispatche| because it looks like there is a problem with this function on Gtest.

Also, you can find new tests for SharedThreadPool, but we don't know if they are releveant for this class.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8432730 [details] [diff] [review]
bug-1002804-tests.patch

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

Please familiarize yourself with and follow the Mozilla C++ style guide: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

These tests need to do more to actually exercise the functionality being tested.

::: content/media/gtest/TestMP4Reader.cpp
@@ +13,5 @@
> +using namespace mozilla;
> +
> +static RefPtr<MP4Decoder> 
> +createMP4Decoder() {
> +	RefPtr<MP4Decoder> mediaDecoder = new MP4Decoder();

Don't bother testing MP4Reader and MP4Decoder. They're can't really cleanly be used without the rest of the system.

::: content/media/gtest/TestMediaTaskQueue.cpp
@@ +50,5 @@
> +    for (uint32_t q = 0; q < numQueues; q++) {
> +      nsRunnable * aRunnable = new InserterTask(arrays[q],i);
> +      EXPECT_NE(nullptr, aRunnable);
> +      // Crash Gtest
> +      //taskQueues[q]->Dispatch(aRunnable);

Why does this crash? If this crashes, then there's no point running this test. We should fix the crash.

What callstack to you get when the crash occurs?

::: content/media/gtest/TestPlatformDecoderModule.cpp
@@ +12,5 @@
> +#include "nsThread.h"
> +#include "nsIRunnable.h"
> +#include "nsStringGlue.h"
> +
> +using namespace mozilla;

Please make the indentation in this file neat and consistent. 2 spaces per indent.

@@ +22,5 @@
> +  {
> +  }
> + 	NS_METHOD Run() MOZ_OVERRIDE {
> + 		PlatformDecoderModule::Init();
> + 		Preferences::SetBool("media.fragmented-mp4.ffmpeg.enabled", true);

We only support ffmpeg on Linux, so it makes no sense to set this pref on non Linux platforms.

On Windows we use Windwos Media Foundation. On MacOSX we'll be landing a PlatformDecoderModule soon, and we're working on a FirefoxOS PlatformDecoderModule too.

So you can't assume the test only runs under Linux.

@@ +54,5 @@
> + 	PlatformDecoderModule * platform = PlatformDecoderModule::Create();
> + 	EXPECT_EQ(nullptr, platform);
> +}
> +
> +

You should test that when you decode encoded H.264 and AAC streams, you actually get decoded samples out.

So, you should copy the approach used in MP4Reader to use our mp4_demuxer to load and demux a file (gizmo.mp4 will do) and check that decoding doesn't fail.

1. Implement mp4_demuxer::Stream to read from gizmo.mp4 off disk.
2. Copy the code in MP4Reader::Init() and MP4Reader::ReadMetadata() to init the demuxer.
3. Demux and decode all samples in the media using the demuxer/PlatformDecoderModule.

@@ +59,5 @@
> +/** 
> +* Create a thread and verify that we cannot create a platform 
> +* in another thread than main thread.
> +*
> +* Compile but process crash when lauching gtest

Presumably the crash is because we fatally assert that we're on the main thread when we create a PlatformDecoderModule.

Don't bother writing a testcase for this. We have assertions to enforce this.

::: content/media/gtest/TestSharedThreadPool.cpp
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "gtest/gtest.h"
> +#include "SharedThreadPool.h"
> +

There is trailing whitespace in this file. Please also indent with 2 spaces per indent, do not use tabs.

@@ +6,5 @@
> +#include "gtest/gtest.h"
> +#include "SharedThreadPool.h"
> +
> +static uint32_t
> +GetThreadLimit(nsCString aName,uint32_t aLimit) {

GetThreadLimit(const nsCString& aName, uint32_t aLimit)

(spaces after ',')

@@ +7,5 @@
> +#include "SharedThreadPool.h"
> +
> +static uint32_t
> +GetThreadLimit(nsCString aName,uint32_t aLimit) {
> +	RefPtr<SharedThreadPool> mPool = SharedThreadPool::Get(aName,aLimit);

Call it "pool", not "mPool". Only use "m" prefix on class members.

@@ +15,5 @@
> +	return i;
> +}
> +
> +static RefPtr<SharedThreadPool>
> +GetSharedThreadPool(nsCString aName, uint32_t aLimit) {

Adding this function adds four lines to the file in order to save typing the "::" characters in SharedThreadPool::Get? Seems like a bad tradeoff to me.

@@ +26,5 @@
> +	}
> +}
> +
> +TEST(SharedThreadPool,Get) {
> +	RefPtr<SharedThreadPool> mPool = GetSharedThreadPool(nsCString("Test 1"),4);

RefPtr<SharedThreadPool> pool = GetSharedThreadPool(NS_LITERAL_CSTRING("Test 1"), 4);

@@ +31,5 @@
> +	
> +	EXPECT_NE(nullptr,mPool);
> +	EXPECT_EQ(mPool,GetSharedThreadPool(nsCString("Test 1"),4));
> +	EXPECT_EQ(mPool,GetSharedThreadPool(nsCString("Test 1"),6));
> +	EXPECT_NE(mPool,GetSharedThreadPool(nsCString("Test 2"),6));

Please also test that SharedThreadPool::Get() twice with the same name returns the same instance of the SharedThreadPool.

Comment 9

[Nicolas Vignes]

I have  a question about MediaTaskQueue and SharedThreadPool. 

Is it possible to have one SharedThreadPool for multiple MediaTaskQueue ?

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

The signature for MediaTaskQueue constructor is MediaTaskQueue(TemporaryRef<SharedThreadPool> aPool). It is possible to pass the same SharedThreadPool instance to different MediaTaskQueue constructors.

Comment 11

[Lucas GOLVEN]

Attachment: bug-1002804.patch

First of all, thanks for your last review. I have tried to improve the patch with your advices. 
I have some questions about what I have done :
- In MediaTaskQueue, I implemented a new version that doesn’t cause a crash failure. Is it sufficient ?
- The function GetSharedThreadPool aims to create a RefPtr in order to allow comparison with nullptr in next gtest assertion. Is it the right way to do that ?
- Another problem is that I didn’t find a nice way to point “gizmo.mp4” in “TestPlatformDecoderModule.cpp”. I am using the absolute path of my machine, in order to understand how the demuxing part works. Is there a Macro to do it easily ?
- In addition for “TestPlatformDecoderModule.cpp”, we have some trouble with the assertion line 139. I can’t manage to find why the |demuxer->Init()| always returns false… It’s like there are no audio and no video in “gizmo.mp4”, which is not true … 
- Also about  “TestPlatformDecoderModule.cpp”, when I comment the assertion  on |demuxer->Init()|, I have an assertion |Don't call on main thread: '!NS_IsMainThread()', file /home/golvenl/mozilla-v2/content/media/MediaResource.cpp, line 1499|. Do I have to create a NsRunnable to run the test or is there another way ?

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8437145 [details] [diff] [review]
bug-1002804.patch

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

I will leave TestPlatformDecoderModule.cpp which I am not familiar with to others' review.

::: content/media/gtest/TestMediaTaskQueue.cpp
@@ +6,5 @@
> +#include "gtest/gtest.h"
> +#include "MediaTaskQueue.h"
> +#include "SharedThreadPool.h"
> +#include "nsCOMPtr.h"
> +#include "nsXPCOMCIDInternal.h"

What is this include for?

@@ +7,5 @@
> +#include "MediaTaskQueue.h"
> +#include "SharedThreadPool.h"
> +#include "nsCOMPtr.h"
> +#include "nsXPCOMCIDInternal.h"
> +#include "nsComponentManagerUtils.h"

I don't see this include is needed.

@@ +29,5 @@
> +  {
> +    mArray.AppendElement(mValue);
> +    return NS_OK;
> +  }
> +  nsTArray<uint32_t>& mArray;

These should be private members, but they don't matter that much.

@@ +33,5 @@
> +  nsTArray<uint32_t>& mArray;
> +  uint32_t mValue;
> +};
> +
> +TEST(MediaTaskQueue, Dispatch)

Can you give comments about what the intention of this test is? It appears to me you are testing if tasks are executed in the order in which they are dispatched.

@@ +37,5 @@
> +TEST(MediaTaskQueue, Dispatch)
> +{
> +  const uint32_t numInserts = 1000;
> +
> +  TemporaryRef<SharedThreadPool> pool = SharedThreadPool::Get(NS_LITERAL_CSTRING("Test"));

You might also want to test various thread limits (ex: 1 ~ 10).

@@ +38,5 @@
> +{
> +  const uint32_t numInserts = 1000;
> +
> +  TemporaryRef<SharedThreadPool> pool = SharedThreadPool::Get(NS_LITERAL_CSTRING("Test"));
> +  mozilla::RefPtr<mozilla::MediaTaskQueue> taskQueues[1];

Why array here? An array with only one element is weird to me. Do you want to test multiple MediaTaskQueues running concurrently?

::: content/media/gtest/TestSharedThreadPool.cpp
@@ +16,5 @@
> +  return i;
> +}
> +
> +static RefPtr<SharedThreadPool>
> +GetSharedThreadPool(nsCString aName, uint32_t aLimit)

It appears to me too simple to be worth a function.

@@ +21,5 @@
> +{
> +  return SharedThreadPool::Get(aName, aLimit);
> +}
> +
> +TEST(SharedThreadPool, ThreadL)

Do you mean ThreadLimit for "ThreadL"?

@@ +31,5 @@
> +  }
> +
> +}
> +
> +TEST(SharedThreadPool, Get)

Please put comments to describe the purpose of this test.

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8437145 [details] [diff] [review]
bug-1002804.patch

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

I will leave TestPlatformDecoderModule.cpp which I am not familiar with to others' review.

::: content/media/gtest/TestMediaTaskQueue.cpp
@@ +6,5 @@
> +#include "gtest/gtest.h"
> +#include "MediaTaskQueue.h"
> +#include "SharedThreadPool.h"
> +#include "nsCOMPtr.h"
> +#include "nsXPCOMCIDInternal.h"

What is this include for?

@@ +7,5 @@
> +#include "MediaTaskQueue.h"
> +#include "SharedThreadPool.h"
> +#include "nsCOMPtr.h"
> +#include "nsXPCOMCIDInternal.h"
> +#include "nsComponentManagerUtils.h"

I don't see this include is needed.

@@ +29,5 @@
> +  {
> +    mArray.AppendElement(mValue);
> +    return NS_OK;
> +  }
> +  nsTArray<uint32_t>& mArray;

These should be private members, but they don't matter that much.

@@ +33,5 @@
> +  nsTArray<uint32_t>& mArray;
> +  uint32_t mValue;
> +};
> +
> +TEST(MediaTaskQueue, Dispatch)

Can you give comments about what the intention of this test is? It appears to me you are testing if tasks are executed in the order in which they are dispatched.

@@ +37,5 @@
> +TEST(MediaTaskQueue, Dispatch)
> +{
> +  const uint32_t numInserts = 1000;
> +
> +  TemporaryRef<SharedThreadPool> pool = SharedThreadPool::Get(NS_LITERAL_CSTRING("Test"));

You might also want to test various thread limits (ex: 1 ~ 10).

@@ +38,5 @@
> +{
> +  const uint32_t numInserts = 1000;
> +
> +  TemporaryRef<SharedThreadPool> pool = SharedThreadPool::Get(NS_LITERAL_CSTRING("Test"));
> +  mozilla::RefPtr<mozilla::MediaTaskQueue> taskQueues[1];

Why array here? An array with only one element is weird to me. Do you want to test multiple MediaTaskQueues running concurrently?

::: content/media/gtest/TestSharedThreadPool.cpp
@@ +21,5 @@
> +{
> +  return SharedThreadPool::Get(aName, aLimit);
> +}
> +
> +TEST(SharedThreadPool, ThreadL)

Do you mean ThreadLimit for "ThreadL"?

@@ +31,5 @@
> +  }
> +
> +}
> +
> +TEST(SharedThreadPool, Get)

Please put comments to describe the purpose of this test.

@@ +35,5 @@
> +TEST(SharedThreadPool, Get)
> +{
> +  RefPtr<SharedThreadPool> pool = GetSharedThreadPool(nsCString("Test 1"), 4);
> +
> +  EXPECT_NE(nullptr, pool);

I personally prefer EXPECT_TRUE and EXPECT_FALSE so I don't have to remember a bunch of EXPECT_EQ/EXPECT_NE/EXPECT_LT/EXPECT_LE/EXPECT_GT/EXPECT_GE.

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

Please ignore comment 12. The review comment was published twice somehow.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Please request review from JW Wang, and address the comments he made above.

I will review the PlatformDecoderModule stuff, but JW will review the rest.

(In reply to Lucas GOLVEN from comment #11)
> Created attachment 8437145 [details] [diff] [review]
> bug-1002804.patch
> 
> First of all, thanks for your last review. I have tried to improve the patch
> with your advices. 
> I have some questions about what I have done :
> - In MediaTaskQueue, I implemented a new version that doesn’t cause a crash
> failure. Is it sufficient ?

We certainly don't want our tests to crash. ;)

If you can think of more ways to test MediaTaskQueue that would be nice. Think about how it could fail or be misused. JW suggested one avenue you could test.


> - The function GetSharedThreadPool aims to create a RefPtr in order to allow
> comparison with nullptr in next gtest assertion. Is it the right way to do
> that ?

You can just go:

RefPtr<SharedThreadPool> pool(SharedThreadPool::Get(name, i));

RefPtr has a constructor that has a TemporaryRef argument:
http://mxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h#237


> - Another problem is that I didn’t find a nice way to point “gizmo.mp4” in
> “TestPlatformDecoderModule.cpp”. I am using the absolute path of my machine,
> in order to understand how the demuxing part works. Is there a Macro to do
> it easily ?

I don't know of any macro, but you should be able to get to it by using a relative path. gizmo.mp4 is in $objdir/_tests/testing/mochitest/tests/content/media/test/gizmo.mp4, I'm not sure where the gtests end up.

> - In addition for “TestPlatformDecoderModule.cpp”, we have some trouble with
> the assertion line 139. I can’t manage to find why the |demuxer->Init()|
> always returns false… It’s like there are no audio and no video in
> “gizmo.mp4”, which is not true … 

Is you TestMP4Stream::ReadAt() being called? Maybe it's returning false when it hits end of stream? You should return true and *aBytesRead may end up less than aCount when end of stream is hit. Also, you could use a plain C FILE* rather than MediaResource like you're doing. The goal here is not to test MediaResource, but to test the PlatformDecoderModule.

> - Also about  “TestPlatformDecoderModule.cpp”, when I comment the assertion 
> on |demuxer->Init()|, I have an assertion |Don't call on main thread:
> '!NS_IsMainThread()', file
> /home/golvenl/mozilla-v2/content/media/MediaResource.cpp, line 1499|. Do I
> have to create a NsRunnable to run the test or is there another way ?

Use a FILE* in your TestMP4Stream instead.

Comment 16

[Lucas GOLVEN]

Attachment: WIP-1002804.patch

Comment 17

[Lucas GOLVEN]

Attachment: bug-1002804.patch

I have found a strange behavior with the ShareThreadPool. I spoke about it with Wang a few hours ago. Below is an explanation about the issue.

The problem is exposed with |TestSharedThreadPool.cpp|, two ShutdownPoolsEvent are waiting on the main Thread.
The first ShutdownPoolsEvent works fine and puts sPools and sMonitor to nullptr. Then the second one fires the MOZ_ASSERT (at line 60 in ShareThreadPool.cpp) because sPools is null. I have tried to describe the issue with a schema in the next attachment.

Wang suggested to delete the assertion. But I think this is dangerous if the sPool is actualy null. I suggest to put something like 
|if (! sMonitor || ! sPools) return;| instead of the assertion.

In addition, I have noticed that there could be another issue in the |DestroySharedThreadPoolHashTable|. I think it might not be thread safe. If the function starts, puts sPools to null, and during it another thread calls ShareThreadPool::Get and tries to add a new element in sPools. This is just a guess, I didn't find a way to confirm this hypothesis. Don't you think that could happen ?

Anyways I have tried to make the modifications suggested in your reviews in the rest of the patch.

PS : I don't know why but you have to run |./mach gtest "gtest_filter:*Shared*"| to have the crash.

Comment 18

[Lucas GOLVEN]

Attachment: schema_issue.png

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Lucas GOLVEN from comment #17)
> Wang suggested to delete the assertion. But I think this is dangerous if the
> sPool is actualy null. I suggest to put something like 
> |if (! sMonitor || ! sPools) return;| instead of the assertion.

I suggest to remove the assertion only if it is a reasonable scenario where sMonitor and sPools are already null and we don't have to delete them again.

To justify a reasonable scenario, we have to find the root cause first.

The scheme in your chart is specious to me. SharedThreadPool::Get will call EnsureInitialized to make sure sMonitor and sPools are created. Therefore when ShutdownPoolsEvent is dispatched in SharedThreadPool::Release, sMonitor and sPools shouldn't be null otherwise you will crash for dereferencing a null pointer.

You should try to fix the assertion first since it blocks the tests.

Comment 20

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Lucas GOLVEN from comment #17)
> In addition, I have noticed that there could be another issue in the
> |DestroySharedThreadPoolHashTable|. I think it might not be thread safe. If
> the function starts, puts sPools to null, and during it another thread calls
> ShareThreadPool::Get and tries to add a new element in sPools. This is just
> a guess, I didn't find a way to confirm this hypothesis. Don't you think
> that could happen ?

No. Both DestroySharedThreadPoolHashTable and SharedThreadPool::Get happen in the main thread. There is no race. Since it is a small test, it should be easy to debug. All you need is add logs to trace when sPools is created/deleted and ShutdownPoolsEvent is dispatched/executed.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8438505 [details] [diff] [review]
bug-1002804.patch

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

I will leave review of TestMediaTaskQueue.cpp and TestSharedThreadPool.cpp to JW.

::: content/media/gtest/TestPlatformDecoderModule.cpp
@@ +23,5 @@
> +  }
> +
> +  virtual void Output(MediaData* aSample) MOZ_OVERRIDE
> +  {
> +    EXPECT_EQ(aSample->mType, mType);

You need to delete aSample here.

@@ +125,5 @@
> +  bool rv = demuxer->Init();
> +  EXPECT_TRUE(rv);
> +}
> +
> +#ifdef XP_UNIX

We also want to run this test on Windows (once gtest works there), not just Linux.

So make this:

#ifdef MOZ_FMP4

@@ +138,5 @@
> +
> +
> +TEST(Platform, AACDecoder)
> +{
> +  PlatformDecoderModule::Init();

Please test that we can demux and decode to the end of the stream, for both AAC and H.264.

You should count the number of samples input and output, and push input if either (($numSamplesInput - $numSamplesOutput) < 3) or if the InputExhausted callback is called.

Comment 22

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8438505 [details] [diff] [review]
bug-1002804.patch

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

::: content/media/gtest/TestMediaTaskQueue.cpp
@@ +29,5 @@
> +};
> +
> +/**
> + * The aim of this test is to check if all the Tasks are
> + * executed in the same order than they are dispatched.

You should also test the case where multiple MediaTaskQueues referring to the same SharedThreadPool are dispatching tasks interleavedly.

@@ +31,5 @@
> +/**
> + * The aim of this test is to check if all the Tasks are
> + * executed in the same order than they are dispatched.
> + */
> +TEST(MediaTaskQueue, Dispatch)

The test name is too generic. Maybe you want to call it "TestDispatchOrder".

@@ +37,5 @@
> +  const uint32_t numInserts = 1000;
> +
> +  for (uint32_t i = 1 ; i <= 10 ; i++) {
> +    TemporaryRef<SharedThreadPool> pool = SharedThreadPool::Get(NS_LITERAL_CSTRING("Test"), i);
> +    mozilla::RefPtr<mozilla::MediaTaskQueue> taskQueues;

s/taskQueues/taskQueue/

@@ +40,5 @@
> +    TemporaryRef<SharedThreadPool> pool = SharedThreadPool::Get(NS_LITERAL_CSTRING("Test"), i);
> +    mozilla::RefPtr<mozilla::MediaTaskQueue> taskQueues;
> +    taskQueues = new mozilla::MediaTaskQueue(pool);
> +
> +    nsTArray<uint32_t> arrays;

s/arrays/array/

@@ +42,5 @@
> +    taskQueues = new mozilla::MediaTaskQueue(pool);
> +
> +    nsTArray<uint32_t> arrays;
> +    for (uint32_t i = 0; i < numInserts; i++) {
> +      taskQueues->Dispatch(new InserterTask(arrays, i));

This is a bad idiom for the memory will be lost when it fails to dispatch. Do this:
RefPtr<nsIRunnable> task = new InserterTask(arrays, i);
taskQueues->Dispatch(task);

::: content/media/gtest/TestSharedThreadPool.cpp
@@ +11,5 @@
> +{
> +  RefPtr<SharedThreadPool> pool = SharedThreadPool::Get(aName, aLimit);
> +
> +  uint32_t i = 0;
> +  pool->GetThreadLimit(&i);

Always check the return value for the error code.

@@ +15,5 @@
> +  pool->GetThreadLimit(&i);
> +  return i;
> +}
> +
> +TEST(SharedThreadPool, ThreadLimits)

Describe the purpose of this test.

@@ +33,5 @@
> + */
> +TEST(SharedThreadPool, Get)
> +{
> +  RefPtr<SharedThreadPool> pool = SharedThreadPool::Get(NS_LITERAL_CSTRING("Test 1"), 4);
> +  RefPtr<SharedThreadPool> tmp1 = nullptr;

You don't need to pass a null. The default constructor of RefPtr will do this for you.

@@ +37,5 @@
> +  RefPtr<SharedThreadPool> tmp1 = nullptr;
> +  RefPtr<SharedThreadPool> tmp2 = nullptr;
> +
> +  EXPECT_NE(nullptr, pool);
> +  

tailing space.

@@ +58,5 @@
> + * The aim of this test is to check that the SharedThreadPool::Get
> + * correctly changes the ThreadLimit of the object when this limit
> + * is greater than the latest limit
> + */
> +TEST(SharedThreadPool, GetAndLimit)

The test name is strange. Maybe you can call it "ChangeThreadLimit".

Comment 23

[Lucas GOLVEN]

Attachment: bug-1002804-fix.patch

I am not sure that this is how you wanted to fix the issue on SharedThreadPool. I didn't find a better place to inhibat the ShutdownPoolsEvent. 
The tests, I have written, now work with this fix.

Please see on the new atachement new verion of this tests.

Comment 24

[Lucas GOLVEN]

Attachment: bug-1002804-tests.patch

Here is new version of Gtests for MediaTaskQueue and SharedThreadPool (not included the one for PlatformDecoderModule, it's still in progress).

I have tried to add all improvements that you suggested.

Comment 25

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8439428 [details] [diff] [review]
bug-1002804-fix.patch

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

The assertion you hit might be a red herring or an indication of a real bug in SharedThreadPool. You need to find out the root cause to tell which it is instead of working around the assertion.

Comment 26

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8439437 [details] [diff] [review]
bug-1002804-tests.patch

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

The tests are good to go as long as you find the root cause of the assertion. You might have to change the test code again if it turns out to be a bug in the test code. Keep the good work.

::: content/media/gtest/TestMediaTaskQueue.cpp
@@ +73,5 @@
> +
> +  nsTArray<uint32_t> arrays[numQueues];
> +
> +  for (uint32_t j = 0; j < numQueues; j++) {
> +    for (uint32_t i = 0; i < numInserts; i++) {

You should swap the 2 for loops so that tasks are dispatched interleavedly to different queues:
Q1 dispatch
Q2 dispatch
Q1 dispatch
Q2 dispatch
...

@@ +75,5 @@
> +
> +  for (uint32_t j = 0; j < numQueues; j++) {
> +    for (uint32_t i = 0; i < numInserts; i++) {
> +      RefPtr<InserterTask> task = new InserterTask(arrays[j], i);
> +      EXPECT_TRUE(NS_SUCCEEDED(taskQueues[j]->Dispatch(task)));

When dispatching a task fails, check of array values will also be wrong. Since such a failure is unlikely to happen, I would suggest just exit the test as a simple error handling.

Comment 27

[Lucas GOLVEN]

Attachment: bug-1002804-tests.patch

Comment 28

[Lucas GOLVEN]

Attachment: bug-1002804-fix.patch

My guess is that the root of the problem on SharedThreadPool is that we may dispatch to the main thread multiple ShutdownEvent which can create problem when both events are executed one after the other. So I implement a boolean variable in order to avoid multiple ShutdownEvent on the main thread. I add a new monitor to protect against multiple access in order to be thread safe. It seems to work well on different tests using SharedThreadPool.

Comment 29

[Lucas GOLVEN]

(In reply to JW Wang [:jwwang] from comment #26)
> Comment on attachment 8439437 [details] [diff] [review]
> bug-1002804-tests.patch
> 
> Review of attachment 8439437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The tests are good to go as long as you find the root cause of the
> assertion. You might have to change the test code again if it turns out to
> be a bug in the test code. Keep the good work.
> 
> ::: content/media/gtest/TestMediaTaskQueue.cpp
> @@ +73,5 @@
> > +
> > +  nsTArray<uint32_t> arrays[numQueues];
> > +
> > +  for (uint32_t j = 0; j < numQueues; j++) {
> > +    for (uint32_t i = 0; i < numInserts; i++) {
> 
> You should swap the 2 for loops so that tasks are dispatched interleavedly
> to different queues:
> Q1 dispatch
> Q2 dispatch
> Q1 dispatch
> Q2 dispatch
> ...

I made the correction on TestMediaTaskQueue in order to dispatch the tasks interleavedly. It works as well as the old version.

> @@ +75,5 @@
> > +
> > +  for (uint32_t j = 0; j < numQueues; j++) {
> > +    for (uint32_t i = 0; i < numInserts; i++) {
> > +      RefPtr<InserterTask> task = new InserterTask(arrays[j], i);
> > +      EXPECT_TRUE(NS_SUCCEEDED(taskQueues[j]->Dispatch(task)));
> 
> When dispatching a task fails, check of array values will also be wrong.
> Since such a failure is unlikely to happen, I would suggest just exit the
> test as a simple error handling.

I’m not sure, but when an assert fail on a test, the test automatically exit, isn’t it ?

Comment 30

[Lucas GOLVEN]

Chris : As you suggest, I added the demux and decode on the TestPlatformDecoderModule copying the approach used in MP4Reader on gizmo.mp4, it looks like it work fine. I checked that we reached the end of the stream. 


Please, is it possible to push this code on the “Try” in order to detect problems on other platform like Windows ? I already had some problems with compilation on an other patch...

Comment 31

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Lucas GOLVEN from comment #29)
> I’m not sure, but when an assert fail on a test, the test automatically
> exit, isn’t it ?

GTest will continue even if EXPECT_TRUE receives a false. You can use ASSERT_TRUE to tell GTest to abort the test when there is a test failure.

Ps, you can click 'review' to add review comments which is easier to read than reply.

Pps, you can split the patch into 2 parts so that Chris and me can review individually.

Comment 32

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8440384 [details] [diff] [review]
bug-1002804-tests.patch

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

R+ as long as the assertion in comment 17 is fixed.

::: content/media/gtest/TestMediaTaskQueue.cpp
@@ +29,5 @@
> +};
> +
> +/**
> + * The aim of this test is to check if all the Tasks are
> + * executed in the same order than they are dispatched.

s/than/in which/

@@ +45,5 @@
> +
> +    nsTArray<uint32_t> array;
> +    for (uint32_t i = 0; i < numInserts; i++) {
> +      RefPtr<InserterTask>  task = new InserterTask(array, i);
> +      EXPECT_TRUE(NS_SUCCEEDED(taskQueue->Dispatch(task)));

http://code.google.com/p/googletest/wiki/V1_7_Primer#Basic_Assertions
You should abort the test when dispatching tasks fails otherwise checking array values will also be wrong.

Comment 33

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8440385 [details] [diff] [review]
bug-1002804-fix.patch

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

I figured out the root cause of this assertion.

1. SharedThreadPool 1 is created via SharedThreadPool::Get.
2. EnsureInitialized is called to create |sMonitor| and |sPools|.
3. SharedThreadPool 1 is deleted.
4. ShutdownPoolsEvent is dispatched to the main thread for sPools->Count() is 0 in SharedThreadPool::Release
5. SharedThreadPool 2 is created via SharedThreadPool::Get.
6. EnsureInitialized is called and returns immediately for |sMonitor| and |sPools| are not null. (ShutdownPoolsEvent is still pending)
7. SharedThreadPool 2 is deleted.
8. Another ShutdownPoolsEvent is dispatched to the main thread for sPools->Count() is 0 in SharedThreadPool::Release (now we have 2 pending ShutdownPoolsEvent)
9. DestroySharedThreadPoolHashTable reset |sPools| and |sMonitor| in the first ShutdownPoolsEvent.
10. We hit assertion in DestroySharedThreadPoolHashTable while running 2nd ShutdownPoolsEvent for |sPools| and |sMonitor| are already reset in the 1st ShutdownPoolsEvent.

The problem is when we try to dispatch a ShutdownPoolsEvent, there might be already a pending ShutdownPoolsEvent. The patch does fix the assertion we hit. However, I am thinking about changing StaticAutoPtr to StaticRefPtr for |sPools| and |sMonitor| so that the life cycle management is symmetric where creation/deletion of SharedThreadPool will always addref/release |sPools| and |sMonitor|.

Comment 34

[Lucas GOLVEN]

Attachment: bug-1002804-tests-MediaTaskQueue-SharedThreadPool.patch

Thanks for your last review. I have done the changes you advice me to do.

I also slip the patch for gtest in two like you said : one that's going to be review by you and the other one for Chris(see next attachement).

Comment 35

[Lucas GOLVEN]

Attachment: bug-1002804-tests-PlatformDecoderModule.patch

Comment 36

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8441633 [details] [diff] [review]
bug-1002804-tests-MediaTaskQueue-SharedThreadPool.patch

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

Looks good. Since I am not a peer, we need Chris for the final review.
Btw, once you fix the assertion, we need another test case for that to test it won't crash or leak with SharedThreadPool created/destroyed in arbitrary order and in arbitrary threads. (Of cos it can only be created in main thread.)

Comment 37

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8441634 [details] [diff] [review]
bug-1002804-tests-PlatformDecoderModule.patch

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

Please test that the entire media can be decoded. Right now, you're only testing that a single frame/audio sample can be decoded.

::: content/media/gtest/TestPlatformDecoderModule.cpp
@@ +17,5 @@
> +
> +class DecoderCallback;
> +
> +/**
> + * Decoder date adapted from MP4Reader.

DecoderData adapted from MP4Reader.

@@ +48,5 @@
> +  uint64_t mNumSamplesInput;
> +  uint64_t mNumSamplesOutput;
> +  uint32_t mDecodeAhead;
> +  // Whether this stream exists in the media.
> +  bool mActive;

You don't need mActive, since you know in advance the file you're reading.

@@ +99,5 @@
> +  DecoderData* mdata;
> +};
> +
> +/**
> + * Function that Decode an MP4 file adapted from MP4Reader::decode.

This comment is misleading. This function decodes a single sample, not a whole file.

@@ +260,5 @@
> +#ifdef XP_UNIX
> +  Preferences::SetBool("media.fragmented-mp4.ffmpeg.enabled", true);
> +#endif
> +  PlatformDecoderModule::Init();
> +  PlatformDecoderModule* platform = PlatformDecoderModule::Create();

nsAutoPtr<PlatformDecoderModule> platform(PlatformDecoderModule::Create());

So it gets deleted when you exit here. Do this also when you use the PlatformDecoderModules below too.

@@ +262,5 @@
> +#endif
> +  PlatformDecoderModule::Init();
> +  PlatformDecoderModule* platform = PlatformDecoderModule::Create();
> +
> +  EXPECT_TRUE(platform);

call platform->Shtudown() here too.

Do this also when you use the PlatformDecoderModules below too.

Comment 38

[Jean-Yves Avenard [:jya]]

MediaTaskQueue now lives in xpcom and was rename TaskQueue

It's likely none of the patches provided above are relevant today, MediaTaskQueue is no longer cancel-able etc..

Comment 39

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:jimm, could you have a look please?
For more information, please visit auto_nag documentation.