1224889 - Implement OpenMax IL H264 video decoding client.

Status: RESOLVED FIXED

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

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: video_omx_pdm

This patch just works; however, it has to do some re-factory works because it is written under tiredness jet lag in WW.

Comment 2

[Alfredo Yang (:alfredo)]

Attachment: video_omx_pdm

A better draft.

Comment 3

[Alfredo Yang (:alfredo)]

Attachment: video_omx_pdm

Comment 4

[Alfredo Yang (:alfredo)]

Attachment: video_omx_pdm

Summary:
1. Add GraphicBuffer to GonkBufferData.
2. Allocate/release different omx buffers (local IMemory buffer, remote IMemory buffer and GraphicBuffer) in GonkOmxPlatformLayer.
3. Add OMX video configure in OmxDataDecoder.

Comment 5

[Sotaro Ikeda [:sotaro]]

Hmm, I saw a crash of 'MOZ_RELEASE_ASSERT(inbuf->mBuffer->nAllocLen >= data->Size());' just by starting the video playback via http. But I could not reproduce it anymore. It is wired.

Comment 6

[Sotaro Ikeda [:sotaro]]

The following STR seems easier to reproduce the problem.

[1] start brouser app.
[2] access the videos's parent url
[3] choose the video and wait video playback
[4] if the crash does not happen, seek the video to somewhere and quickly push back button of the browser

if the crash did not happen on [4] back to [3] until the crash happen.

Comment 7

[Sotaro Ikeda [:sotaro]]

When the crash happened, input buffer size of video omx il was 8192 and data->Size() was 58934. the input buffer size of video omx il seems too small. The size is same to input buffer of audio omx il.

Comment 8

[Sotaro Ikeda [:sotaro]]

Normally, input buffer size of video omx il with the video was 786432.

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> The following STR seems easier to reproduce the problem.
> 
> [1] start brouser app.
> [2] access the videos's parent url
> [3] choose the video and wait video playback
> [4] if the crash does not happen, seek the video to somewhere and quickly
> push back button of the browser
> 
> if the crash did not happen on [4] back to [3] until the crash happen.

It might be difficult to reproduce the crash if we add may logs.

Comment 10

[Alfredo Yang (:alfredo)]

(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> When the crash happened, input buffer size of video omx il was 8192 and
> data->Size() was 58934. the input buffer size of video omx il seems too
> small. The size is same to input buffer of audio omx il.

It should be from h264 software decoder. But I don't understand why it requests such small buffer size.

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Alfredo Yang (:alfredo) from comment #10)
> (In reply to Sotaro Ikeda [:sotaro] from comment #7)
> > When the crash happened, input buffer size of video omx il was 8192 and
> > data->Size() was 58934. the input buffer size of video omx il seems too
> > small. The size is same to input buffer of audio omx il.
> 
> It should be from h264 software decoder. But I don't understand why it
> requests such small buffer size.

Shouldn't we disable h264 omx il video decoder except emulator case, should we? It cannot decode correctly in high profile videos.

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> 
> Shouldn't we disable h264 omx il video decoder except emulator case, should
> we? It cannot decode correctly in high profile videos.

It means android default h.264 decoder.

Comment 13

[Alfredo Yang (:alfredo)]

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> (In reply to Alfredo Yang (:alfredo) from comment #10)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #7)
> > > When the crash happened, input buffer size of video omx il was 8192 and
> > > data->Size() was 58934. the input buffer size of video omx il seems too
> > > small. The size is same to input buffer of audio omx il.
> > 
> > It should be from h264 software decoder. But I don't understand why it
> > requests such small buffer size.
> 
> Shouldn't we disable h264 omx il video decoder except emulator case, should
> we? It cannot decode correctly in high profile videos.

Just h264 software codec or all software codecs?

Comment 14

[Sotaro Ikeda [:sotaro]]

As you assumed, if I fixed as to use only sw video codec, the video playback failed with the crash.

Comment 15

[Alfredo Yang (:alfredo)]

Attachment: video_omx_pdm

1. release resource at DoAsyncShutdown().
2. correct assertion.

Comment 16

[Sotaro Ikeda [:sotaro]]

Conformation of the patch seems to be blocked by Bug 1232348. It makes the video playback unstable.

Comment 17

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8701736 [details] [diff] [review]
video_omx_pdm

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

The code seems very nice. The followings are some comments.

::: dom/media/platforms/omx/GonkOmxPlatformLayer.cpp
@@ +10,5 @@
>  #include "MediaInfo.h"
> +#include "ImageContainer.h"
> +#include "mozilla/Monitor.h"
> +#include "mozilla/layers/TextureClient.h"
> +#include "mozilla/layers/TextureClientRecycleAllocator.h"

Nit. This include is not necessary.

@@ +218,5 @@
> +  LOG("Recycle Buffer %p", buffer);
> +
> +  // Add extra reference to avoid GonkBufferData been destroyed after dispatching
> +  // to Omx task queue.
> +  buffer->mKungfuDeathGrips = buffer;

TextureClient already has similar mechanism to do it. TextureClient::SetRecycleAllocar() does it. I am not fun of doing one thing by different ways. Can't we use same mechanism?

@@ +401,5 @@
> +  status_t st;
> +  // TODO: layer keeps GraphicBuffer for a while, it is possible some extra
> +  //       buffers are required to get better performance. This value needs
> +  //       to be confirmed later.
> +  int minUndequeuedBuffers = 2;

Can't we change the name? "Undequeued" means that undequeue buffer from ANativeWindow to keep the buffers undequeued in ANativeWindow. ANativeWindow is buffer source and render target in android. The situation is different in this class.

And increasing it means we use more gralloc than android. I do not think that it is a good idea to increase it casually. And on ICS fxos devices, if we increased it, it caused omx il video decoder crash.

::: dom/media/platforms/omx/OmxDataDecoder.cpp
@@ +336,5 @@
> +
> +  aBufferData->mStatus = BufferData::BufferStatus::OMX_CLIENT_OUTPUT;
> +
> +  // In gonk video displaying, layers will call RecycleCallback when frame is
> +  // displayed. And then promise will be resolved in the callback.

Can you update the comment? TextureClient's recycle callback is called when a reference count of TextureClient becomes 1. We expect that the last ref count is held by TextureClient allocator(recycler). We use this mechanism for media playback not only on gonk, but also on Windows. D3D11RecycleAllocator and D3D9RecycleAllocator do TextureClient recycling that are used by DXVA2Manager.

@@ +347,5 @@
> +  //   layer doesn't call recycle buffer.
> +  // TODO:
> +  //   It could be risky if a promise is initialized here but for some reasons
> +  //   reader or other module doesn't send this frame to display. Then it will
> +  //   cause a unresolved promise waiting infinitely in CollectBufferPromises().

You might want to create bugs for them and write the bugs numbers. Normally if we add TODO, we create a bug.

Comment 18

[Alfredo Yang (:alfredo)]

(In reply to Sotaro Ikeda [:sotaro PTO 28/Dec - 4/Jan] from comment #17)
> Comment on attachment 8701736 [details] [diff] [review]
> video_omx_pdm
> 
> Review of attachment 8701736 [details] [diff] [review]:
> -----------------------------------------------------------------
> And increasing it means we use more gralloc than android. I do not think
> that it is a good idea to increase it casually. And on ICS fxos devices, if
> we increased it, it caused omx il video decoder crash.
> 
> ::: dom/media/platforms/omx/OmxDataDecoder.cpp
> @@ +336,5 @@
> > +
> > +  aBufferData->mStatus = BufferData::BufferStatus::OMX_CLIENT_OUTPUT;
> > +
> > +  // In gonk video displaying, layers will call RecycleCallback when frame is
> > +  // displayed. And then promise will be resolved in the callback.
> 
> Can you update the comment? TextureClient's recycle callback is called when
> a reference count of TextureClient becomes 1. We expect that the last ref
> count is held by TextureClient allocator(recycler). We use this mechanism
> for media playback not only on gonk, but also on Windows.
> D3D11RecycleAllocator and D3D9RecycleAllocator do TextureClient recycling
> that are used by DXVA2Manager.

Yes, I'll update the comments.

BTW, I've discussed it with :jolin when I wrote this patch, count on reference to call RecycleCallback sounds implicit and could be easy mis-calling RecycleCallback by modules use RefPtr to hold it. IMHO, it could be better that layers could call RecycleCallback() with explicit way instead of reference count to 1?
Another problem is: it is not thread safe in ClearRecycleCallback() and RecycleCallback(). I encountered race condition when I wrote this patch at early version. To solve it, I need to add reference count in GonkBufferData::ClearRecycleCallback() before calling ClearRecycleCallback. That's a little tricky for me.

> 
> @@ +347,5 @@
> > +  //   layer doesn't call recycle buffer.
> > +  // TODO:
> > +  //   It could be risky if a promise is initialized here but for some reasons
> > +  //   reader or other module doesn't send this frame to display. Then it will
> > +  //   cause a unresolved promise waiting infinitely in CollectBufferPromises().
> 
> You might want to create bugs for them and write the bugs numbers. Normally
> if we add TODO, we create a bug.

Thanks for comments, I'll update others.

Comment 19

[Alfredo Yang (:alfredo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f39625f98123

Comment 20

[Alfredo Yang (:alfredo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2478e0a100d

Comment 21

[Alfredo Yang (:alfredo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdeab8eb768b

Comment 22

[John Lin [:jhlin][:jolin]]

Comment on attachment 8701736 [details] [diff] [review]
video_omx_pdm

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

::: dom/media/platforms/omx/GonkOmxPlatformLayer.cpp
@@ +328,5 @@
>    bool liveinlocal = mOmx->livesLocally(mNode, getpid());
>  
>    // MemoryDealer is a IPC buffer allocator in Gonk because IOMX is actually
>    // lives in mediaserver.
>    mMemoryDealer[aType] = new MemoryDealer(t, "Gecko-OMX");

When !useGraphicBuffer, this line will try to allocate a 0-size heap and cause error:
  E MemoryHeapBase: mmap(fd=63, size=0) failed (Invalid argument)

How about moving it to the else block above (line#326)?

Comment 23

[Alfredo Yang (:alfredo)]

(In reply to John Lin [:jolin][:jhlin] from comment #22)
> Comment on attachment 8701736 [details] [diff] [review]
> video_omx_pdm
> 
> Review of attachment 8701736 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/omx/GonkOmxPlatformLayer.cpp
> @@ +328,5 @@
> >    bool liveinlocal = mOmx->livesLocally(mNode, getpid());
> >  
> >    // MemoryDealer is a IPC buffer allocator in Gonk because IOMX is actually
> >    // lives in mediaserver.
> >    mMemoryDealer[aType] = new MemoryDealer(t, "Gecko-OMX");
> 
> When !useGraphicBuffer, this line will try to allocate a 0-size heap and
> cause error:
>   E MemoryHeapBase: mmap(fd=63, size=0) failed (Invalid argument)
> 
> How about moving it to the else block above (line#326)?

Yes, you also need to check mMemoryDealer in GonkOmxPlatformLayer::ReleaseOmxBuffer().

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to Alfredo Yang (:alfredo) from comment #18)
> 
> BTW, I've discussed it with :jolin when I wrote this patch, count on
> reference to call RecycleCallback sounds implicit and could be easy
> mis-calling RecycleCallback by modules use RefPtr to hold it. IMHO, it could
> be better that layers could call RecycleCallback() with explicit way instead
> of reference count to 1?

How do you do it?

Comment 25

[Sotaro Ikeda [:sotaro]]

You might want to add the following handling.
  ro.moz.omx.hw.max_height
  ro.moz.omx.hw.max_width

Comment 26

[Sotaro Ikeda [:sotaro]]

(In reply to Alfredo Yang (:alfredo) from comment #18)
> 
> BTW, I've discussed it with :jolin when I wrote this patch, count on
> reference to call RecycleCallback sounds implicit and could be easy
> mis-calling RecycleCallback by modules use RefPtr to hold it. IMHO, it could
> be better that layers could call RecycleCallback() with explicit way instead
> of reference count to 1?

Sorry, I do not understand well about your concern. Can you explain more about "mis-calling RecycleCallback by modules use RefPtr to hold it" ?

Comment 27

[Alfredo Yang (:alfredo)]

(In reply to Sotaro Ikeda [:sotaro PTO 28/Dec - 4/Jan] from comment #26)
> (In reply to Alfredo Yang (:alfredo) from comment #18)
> > 
> > BTW, I've discussed it with :jolin when I wrote this patch, count on
> > reference to call RecycleCallback sounds implicit and could be easy
> > mis-calling RecycleCallback by modules use RefPtr to hold it. IMHO, it could
> > be better that layers could call RecycleCallback() with explicit way instead
> > of reference count to 1?
> 
> Sorry, I do not understand well about your concern. Can you explain more
> about "mis-calling RecycleCallback by modules use RefPtr to hold it" ?

I mean reply on reference count to 1 is too implicit and potential dangerous. We need to ensure layers is the last holder; otherwise this mechanism will be failed.
Anyway, I found a way to work with TextureClientRecycleAllocator and using TextureClientHolder instead of TextureClient. I'll update the patch soon.

Comment 28

[John Lin [:jhlin][:jolin]]

(In reply to Alfredo Yang (:alfredo) from comment #23)
> (In reply to John Lin [:jolin][:jhlin] from comment #22)
> > Comment on attachment 8701736 [details] [diff] [review]
> > video_omx_pdm
> > 
> > Review of attachment 8701736 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/media/platforms/omx/GonkOmxPlatformLayer.cpp
> > @@ +328,5 @@
> > >    bool liveinlocal = mOmx->livesLocally(mNode, getpid());
> > >  
> > >    // MemoryDealer is a IPC buffer allocator in Gonk because IOMX is actually
> > >    // lives in mediaserver.
> > >    mMemoryDealer[aType] = new MemoryDealer(t, "Gecko-OMX");
> > 
> > When !useGraphicBuffer, this line will try to allocate a 0-size heap and
> > cause error:
> >   E MemoryHeapBase: mmap(fd=63, size=0) failed (Invalid argument)
> > 
> > How about moving it to the else block above (line#326)?
> 
> Yes, you also need to check mMemoryDealer in
> GonkOmxPlatformLayer::ReleaseOmxBuffer().

 For |mMemoryDealer[aType].clear();|? IMHO it's not absolutely necessary since calling clear() on a null sp<> seems harmless [1].

 [1] https://android.googlesource.com/platform/system/core/+/kitkat-release/include/utils/StrongPointer.h#195

Comment 29

[John Lin [:jhlin][:jolin]]

Comment on attachment 8701736 [details] [diff] [review]
video_omx_pdm

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

::: dom/media/platforms/omx/GonkOmxPlatformLayer.cpp
@@ +170,5 @@
> +GonkBufferData::InitGraphicBuffer(OMX_VIDEO_PORTDEFINITIONTYPE& aDef)
> +{
> +  // Allocate Gralloc texture memory.
> +  layers::GrallocTextureData* texData =
> +    layers::GrallocTextureData::Create(gfx::IntSize(aDef.nStride, aDef.nSliceHeight),

Use aDef.nStride and aDef.nSliceHeight could be problematic for QCOM Venus buffers because these are the actual buffer size (with padding pixels). Using the original size (width/height of VideoInfo::mImage) seems better.

Comment 30

[Alfredo Yang (:alfredo)]

(In reply to Sotaro Ikeda [:sotaro PTO 28/Dec - 4/Jan] from comment #17)
> Comment on attachment 8701736 [details] [diff] [review]
> video_omx_pdm
> 
> Review of attachment 8701736 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +218,5 @@
> > +  LOG("Recycle Buffer %p", buffer);
> > +
> > +  // Add extra reference to avoid GonkBufferData been destroyed after dispatching
> > +  // to Omx task queue.
> > +  buffer->mKungfuDeathGrips = buffer;
> 
> TextureClient already has similar mechanism to do it.
> TextureClient::SetRecycleAllocar() does it. I am not fun of doing one thing
> by different ways. Can't we use same mechanism?

I have tried several ways to use TextureClientRecycleAllocator but there are always problems or complicated the code logic.
Current problems I encounter are:

1. How to get GraphicBuffer from TextureClient?

2. OMX needs buffers to be registered before OMX_Executing but TextureClientRecycleAllocator::CreateOrRecycle could return a new unregistered buffer. That'll cause problem.

3. TextureClientRecycleAllocator are sync API. We can't know when the TextureClient is free to use again except for keep polling it.

Any suggestions? Thank you.

Comment 31

[Alfredo Yang (:alfredo)]

(In reply to John Lin [:jolin][:jhlin] from comment #29)
> Comment on attachment 8701736 [details] [diff] [review]
> video_omx_pdm
> 
> Review of attachment 8701736 [details] [diff] [review]:
> -----------------------------------------------------------------
> Use aDef.nStride and aDef.nSliceHeight could be problematic for QCOM Venus
> buffers because these are the actual buffer size (with padding pixels).
> Using the original size (width/height of VideoInfo::mImage) seems better.

Do you have test video? I feel nervous if we provide the smaller buffer.

Comment 32

[Alfredo Yang (:alfredo)]

Attachment: video_omx_pdm

I manage to have a patch with TextureClientRecycleAllocator, although I add some APIs for async operation. I'm not sure if it complies with your design of TextureClientRecycleAllocator.

Comment 33

[John Lin [:jhlin][:jolin]]

(In reply to Alfredo Yang (:alfredo) from comment #31)
> (In reply to John Lin [:jolin][:jhlin] from comment #29)
> > Comment on attachment 8701736 [details] [diff] [review]
> > video_omx_pdm
> > 
> > Review of attachment 8701736 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > Use aDef.nStride and aDef.nSliceHeight could be problematic for QCOM Venus
> > buffers because these are the actual buffer size (with padding pixels).
> > Using the original size (width/height of VideoInfo::mImage) seems better.
> 
> Do you have test video? I feel nervous if we provide the smaller buffer.

 I found the problem when playing FTU tutorial video: go to "Settings" > "Developer" > "Launch First time use" to check it.

Comment 34

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8703520 [details] [diff] [review]
video_omx_pdm

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

::: dom/media/platforms/omx/GonkOmxPlatformLayer.cpp
@@ +184,5 @@
> +                                  layers::TextureClientRecycleAllocator* aAllocator)
> +{
> +  // Allocate Gralloc texture memory.
> +  layers::GrallocTextureData* textureData =
> +    layers::GrallocTextureData::Create(gfx::IntSize(aDef.nStride, aDef.nSliceHeight),

Is there a reason to choose nStride and nSliceHeight? OMXCodec and ACodec uses video.nFrameWidth and video.nFrameHeight.

Comment 35

[Alfredo Yang (:alfredo)]

(In reply to Sotaro Ikeda [:sotaro] from comment #34)
> Comment on attachment 8703520 [details] [diff] [review]
> video_omx_pdm
> 
> Review of attachment 8703520 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/omx/GonkOmxPlatformLayer.cpp
> @@ +184,5 @@
> > +                                  layers::TextureClientRecycleAllocator* aAllocator)
> > +{
> > +  // Allocate Gralloc texture memory.
> > +  layers::GrallocTextureData* textureData =
> > +    layers::GrallocTextureData::Create(gfx::IntSize(aDef.nStride, aDef.nSliceHeight),
> 
> Is there a reason to choose nStride and nSliceHeight? OMXCodec and ACodec
> uses video.nFrameWidth and video.nFrameHeight.

I follow the spec 4.2.2: "The minimum buffer payload size can be easily calculated as the absolute value of (nSliceHeight * nStride )."

Comment 36

[Sotaro Ikeda [:sotaro]]

(In reply to Alfredo Yang (:alfredo) from comment #35)
> > Is there a reason to choose nStride and nSliceHeight? OMXCodec and ACodec
> > uses video.nFrameWidth and video.nFrameHeight.
> 
> I follow the spec 4.2.2: "The minimum buffer payload size can be easily
> calculated as the absolute value of (nSliceHeight * nStride )."

It make videos of FTU rendered as horizontally stretched. If I set video.nFrameWidth and video.nFrameHeight, the videos rendered correctly on display of flame-kk. The regression needs to be addressed.

Comment 37

[Alfredo Yang (:alfredo)]

(In reply to Sotaro Ikeda [:sotaro] from comment #36)
> (In reply to Alfredo Yang (:alfredo) from comment #35)
> > > Is there a reason to choose nStride and nSliceHeight? OMXCodec and ACodec
> > > uses video.nFrameWidth and video.nFrameHeight.
> > 
> > I follow the spec 4.2.2: "The minimum buffer payload size can be easily
> > calculated as the absolute value of (nSliceHeight * nStride )."
> 
> It make videos of FTU rendered as horizontally stretched. If I set
> video.nFrameWidth and video.nFrameHeight, the videos rendered correctly on
> display of flame-kk. The regression needs to be addressed.

:jolin, do you like to file a bug? Or I can fix it directly in this patch.

Comment 38

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8703520 [details] [diff] [review]
video_omx_pdm

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

How to recycle is very different between TextureClientRecycleAllocator and GonkOmxPlatformLayer. It seems better just share API that is used by TextureClient. I am going to create a rough temporary sample to do it.

::: gfx/layers/client/TextureClientRecycleAllocator.cpp
@@ +278,5 @@
> +
> +  // Clearing the recycle allocator drops a reference, so make sure we stay alive
> +  // for the duration of this function.
> +  RefPtr<TextureClientRecycleAllocator> kungFuDeathGrip(this);
> +  aClient->SetRecycleAllocator(nullptr);

We do not want to do clear RecycleAllocator except in recycle callback to avoid race condition.

Comment 39

[Sotaro Ikeda [:sotaro]]

Attachment: temporary patch

Inter-diff patch to attachment 8703520 [details] [diff] [review]. It is a rough temporary patch just to show my comment. It could be more polished.

Comment 40

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #39)
> Created attachment 8704089 [details] [diff] [review]
> temporary patch
> 
> Inter-diff patch to attachment 8703520 [details] [diff] [review]. It is a
> rough temporary patch just to show my comment. It could be more polished.

Oh, I removed a change to TextureClientRecycleAllocator of attachment 8703520 [details] [diff] [review] before create attachment 8704089 [details] [diff] [review].

Comment 41

[Sotaro Ikeda [:sotaro]]

:alfredo, how about attachment 8704089 [details] [diff] [review] way of implementation?

Comment 42

[Alfredo Yang (:alfredo)]

(In reply to Sotaro Ikeda [:sotaro] from comment #41)
> :alfredo, how about attachment 8704089 [details] [diff] [review] way of
> implementation?

It looks much better. Thank you!
I'll update my patch according it.

Comment 43

[John Lin [:jhlin][:jolin]]

(In reply to Alfredo Yang (:alfredo) from comment #37)
> (In reply to Sotaro Ikeda [:sotaro] from comment #36)
> > (In reply to Alfredo Yang (:alfredo) from comment #35)
> > > > Is there a reason to choose nStride and nSliceHeight? OMXCodec and ACodec
> > > > uses video.nFrameWidth and video.nFrameHeight.
> > > 
> > > I follow the spec 4.2.2: "The minimum buffer payload size can be easily
> > > calculated as the absolute value of (nSliceHeight * nStride )."
> > 
> > It make videos of FTU rendered as horizontally stretched. If I set
> > video.nFrameWidth and video.nFrameHeight, the videos rendered correctly on
> > display of flame-kk. The regression needs to be addressed.
> 
> :jolin, do you like to file a bug? Or I can fix it directly in this patch.

 I'm fine if you fix both this and bug 1235340 directly in this patch. Thanks!

Comment 44

[Alfredo Yang (:alfredo)]

Attachment: video_omx_pdm

1. Update according to comment 17.
2. Use ITextureClientRecycleAllocator.
3. Update incorrect width/height.

Comment 45

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8704504 [details] [diff] [review]
video_omx_pdm

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

Looks better :) nit: the patch failed to build on master hamachi.

The followings are some comments.

::: dom/media/platforms/omx/GonkOmxPlatformLayer.cpp
@@ +159,5 @@
> +    MOZ_ASSERT(mTextureClient);
> +  }
> +
> +  RefPtr<TextureClientRecyclePromise> WaitforRecycle()
> +  {

You might want to assert that this function is not called after calling ReleaseBuffer().

@@ +194,5 @@
> +      });
> +    mTaskQueue->Dispatch(r.forget());
> +  }
> +
> +  void ReleaseBuffer()

It might be better to change the function name like Shutdown() to make clear that the object could not be reused after calling this function.

@@ +204,5 @@
> +      // Add an extra reference count of TextureClient so it won't call
> +      // RecycleCallback during clearing allocator.
> +      // In some case, TextureClient's lifetime could loanger than
> +      // GonkOmxPlatformLayer so the callback needs to be cleared here.
> +      RefPtr<layers::TextureClient> client(mTextureClient);

This code could not protect from RecycleCallback race condition. It could still happen depends on scheduler's scheduling. As in Comment 38, we do not want to clear RecycleAllocator except during recycle callback. Is there a reason why you want to do it here?

@@ +205,5 @@
> +      // RecycleCallback during clearing allocator.
> +      // In some case, TextureClient's lifetime could loanger than
> +      // GonkOmxPlatformLayer so the callback needs to be cleared here.
> +      RefPtr<layers::TextureClient> client(mTextureClient);
> +      client->SetRecycleAllocator(nullptr);

It seems better to clear mTaskQueue here. We do not use mTaskQueue after calling this function.

Comment 46

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8704504 [details] [diff] [review]
video_omx_pdm

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

::: dom/media/platforms/omx/GonkOmxPlatformLayer.cpp
@@ +189,5 @@
> +
> +    RefPtr<GonkTextureClientRecycleHandler> self(this);
> +    nsCOMPtr<nsIRunnable> r =
> +      NS_NewRunnableFunction([self] () {
> +        self->mPromise.ResolveIfExists(self->mTextureClient, __func__);

Is there a reason to resolve promise on the TaskQueue?

Comment 47

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #46)
> Review of attachment 8704504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/omx/GonkOmxPlatformLayer.cpp
> @@ +189,5 @@
> > +
> > +    RefPtr<GonkTextureClientRecycleHandler> self(this);
> > +    nsCOMPtr<nsIRunnable> r =
> > +      NS_NewRunnableFunction([self] () {
> > +        self->mPromise.ResolveIfExists(self->mTextureClient, __func__);
> 
> Is there a reason to resolve promise on the TaskQueue?

I understand, ResolveIfExists() operation is not atomic. Therefore mPromise.RejectIfExists() and mPromise.ResolveIfExists() could cause race condition.

Comment 48

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #47)
> 
> I understand, ResolveIfExists() operation is not atomic. Therefore
> mPromise.RejectIfExists() and mPromise.ResolveIfExists() could cause race
> condition.

One solution to it seems using Monitor and SetMonitor().
  MozPromiseHolder::SetMonitor(Monitor* aMonitor)
  https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/MozPromise.h#742

Comment 49

[Sotaro Ikeda [:sotaro]]

> > I understand, ResolveIfExists() operation is not atomic. Therefore
> > mPromise.RejectIfExists() and mPromise.ResolveIfExists() could cause race
> > condition.
> 
> One solution to it seems using Monitor and SetMonitor().
>   MozPromiseHolder::SetMonitor(Monitor* aMonitor)
>  
> https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/MozPromise.h#742

If we use it, we do not need to use TaskQueue in GonkTextureClientRecycleHandler.

Comment 50

[Jean-Yves Avenard [:jya]]

(In reply to Sotaro Ikeda [:sotaro] from comment #48)
> (In reply to Sotaro Ikeda [:sotaro] from comment #47)
> > 
> > I understand, ResolveIfExists() operation is not atomic. Therefore
> > mPromise.RejectIfExists() and mPromise.ResolveIfExists() could cause race
> > condition.
> 
> One solution to it seems using Monitor and SetMonitor().
>   MozPromiseHolder::SetMonitor(Monitor* aMonitor)
>  
> https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/MozPromise.h#742

This is purely for diagnostic.
SetMonitor does nothing but add a Monitor and then the code always ensure monitor is held or otherwise assert.

It will not lock/unlock as needed.

Comment 51

[Sotaro Ikeda [:sotaro]]

In my understanding, the Monitor needs to be held by the caller during calling ResolveIfExists().

Comment 52

[Jean-Yves Avenard [:jya]]

That's right... Bu PromiseHolder::SetMonitor serves no purpose other than ensuring proper locking. You still need to manually use an external Monitor.

Look there: https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/MozPromise.h#746

all it does is asserting if the monitor isn't held. It won't solve the race condition described earlier.

A MozPromise is thread-safe, it's MozPromiseHolder that isn't.

Comment 53

[Alfredo Yang (:alfredo)]

(In reply to Sotaro Ikeda [:sotaro] from comment #45)
> Comment on attachment 8704504 [details] [diff] [review]
> video_omx_pdm
> 
> Review of attachment 8704504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks better :) nit: the patch failed to build on master hamachi.

IIRC, hamachi is ICS. Do you build hamachi with KK?

IOMX on ICS has slight different comparing to KK.
My original plan is to support ICS later for emulator try. But I've learned from :Blake yesterday that ICS emulator try will be stopped very soon. So I don't have plan to fix it on ICS.
Or do you think there is other reason to fix it?

> @@ +204,5 @@
> > +      // Add an extra reference count of TextureClient so it won't call
> > +      // RecycleCallback during clearing allocator.
> > +      // In some case, TextureClient's lifetime could loanger than
> > +      // GonkOmxPlatformLayer so the callback needs to be cleared here.
> > +      RefPtr<layers::TextureClient> client(mTextureClient);
> 
> This code could not protect from RecycleCallback race condition. It could
> still happen depends on scheduler's scheduling. As in Comment 38, we do not
> want to clear RecycleAllocator except during recycle callback. Is there a
> reason why you want to do it here?

I found GraphicsBuffer will be held after OmxDataDecoder is shutdown. It can be reproduced by Video AP with following steps:

1. Play a video and then pause.
2. press the back button on the left up corner.
3. The RecycleCallback of the paused frame is called after OMX PDM is shutdown.

So it needs to clear the RecycleCallback when shutdown.
I thought AtomicRefCountedWithFinalize is thread safe but it isn't. I'll rewrite this part with lock.

Comment 54

[Sotaro Ikeda [:sotaro]]

(In reply to Alfredo Yang (:alfredo) from comment #53)
> (In reply to Sotaro Ikeda [:sotaro] from comment #45)
> > Comment on attachment 8704504 [details] [diff] [review]
> > video_omx_pdm
> > 
> > Review of attachment 8704504 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks better :) nit: the patch failed to build on master hamachi.
> 
> IIRC, hamachi is ICS. Do you build hamachi with KK?
> 
> IOMX on ICS has slight different comparing to KK.
> My original plan is to support ICS later for emulator try. But I've learned
> from :Blake yesterday that ICS emulator try will be stopped very soon. So I
> don't have plan to fix it on ICS.
> Or do you think there is other reason to fix it?

I talked just about build failure. We cannnot check in the patch if it break the build. And it was very easy to address it.

> 
> > @@ +204,5 @@
> > > +      // Add an extra reference count of TextureClient so it won't call
> > > +      // RecycleCallback during clearing allocator.
> > > +      // In some case, TextureClient's lifetime could loanger than
> > > +      // GonkOmxPlatformLayer so the callback needs to be cleared here.
> > > +      RefPtr<layers::TextureClient> client(mTextureClient);
> > 
> > This code could not protect from RecycleCallback race condition. It could
> > still happen depends on scheduler's scheduling. As in Comment 38, we do not
> > want to clear RecycleAllocator except during recycle callback. Is there a
> > reason why you want to do it here?
> 
> I found GraphicsBuffer will be held after OmxDataDecoder is shutdown. It can
> be reproduced by Video AP with following steps:
> 
> 1. Play a video and then pause.
> 2. press the back button on the left up corner.
> 3. The RecycleCallback of the paused frame is called after OMX PDM is
> shutdown.

I do not think this is a problem. Can you explain about what is the problem of it?

> 
> So it needs to clear the RecycleCallback when shutdown.

I do not find the valid reason to call client->SetRecycleAllocator(nullptr) in ReleaseBuffer().

> I thought AtomicRefCountedWithFinalize is thread safe but it isn't. I'll
> rewrite this part with lock.

Can you explain about why you thought it? We cannot use the lock here. It degrade performance and increase system resource usages.

Comment 55

[Sotaro Ikeda [:sotaro]]

(In reply to Alfredo Yang (:alfredo) from comment #53)
> (In reply to Sotaro Ikeda [:sotaro] from comment #45)
> > Comment on attachment 8704504 [details] [diff] [review]
> > video_omx_pdm
> > 
> > Review of attachment 8704504 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks better :) nit: the patch failed to build on master hamachi.
> 
> IIRC, hamachi is ICS. Do you build hamachi with KK?

hamachi is ICS. We should not break a build when we check in a patch.

Comment 56

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8704504 [details] [diff] [review]
video_omx_pdm

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

::: dom/media/platforms/omx/GonkOmxPlatformLayer.cpp
@@ +204,5 @@
> +      // Add an extra reference count of TextureClient so it won't call
> +      // RecycleCallback during clearing allocator.
> +      // In some case, TextureClient's lifetime could loanger than
> +      // GonkOmxPlatformLayer so the callback needs to be cleared here.
> +      RefPtr<layers::TextureClient> client(mTextureClient);

This part break the assumption of AtomicRefCountedWithFinalize. We should not increase the ref count of TextureClient by using a ref counted pointer of TextureClient that is held by ITextureClientRecycleAllocator if it is not recycled yet.

Comment 57

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #56)
> 
> This part break the assumption of AtomicRefCountedWithFinalize. We should
> not increase the ref count of TextureClient by using a ref counted pointer
> of TextureClient that is held by ITextureClientRecycleAllocator if it is not
> recycled yet.

When we does recycling with AtomicRefCountedWithFinalize. refcount 1 is lowest refcount number. We can copy ref counted pointer without care except a pointer that is held by ITextureClientRecycleAllocator.

Comment 58

[Alfredo Yang (:alfredo)]

(In reply to Sotaro Ikeda [:sotaro] from comment #57)
> (In reply to Sotaro Ikeda [:sotaro] from comment #56)
> > 
> > This part break the assumption of AtomicRefCountedWithFinalize. We should
> > not increase the ref count of TextureClient by using a ref counted pointer
> > of TextureClient that is held by ITextureClientRecycleAllocator if it is not
> > recycled yet.
> 
> When we does recycling with AtomicRefCountedWithFinalize. refcount 1 is
> lowest refcount number. We can copy ref counted pointer without care except
> a pointer that is held by ITextureClientRecycleAllocator.

I was wrong on IRC talk.
The GonkTextureClientRecycleHandler is alive after PDM is destroyed. However, the OMX TaskQueue doesn't accept any new task at that point because of [1]. So the runnable to resolve the promise in GonkTextureClientRecycleHandler::RecycleTextureClient is failed to dispatch.

There are 2 solutions I think right now:
1. Remove the OMX TaskQueue shutdown codes in [1], it works well.
2. Add an Atomic<bool> mShutdown in GonkTextureClientRecycleHandler, so we can avoid dispatching runnable when OMX TaskQueue is shutdown in GonkTextureClientRecycleHandler::RecycleTextureClient. However, I'm not sure if it impacts graphics performance.

[1] https://dxr.mozilla.org/mozilla-central/rev/1ec3a3ff68f2d1a54e6ed33e926c28fee286bdf1/dom/media/platforms/omx/OmxDataDecoder.cpp#225

Comment 59

[Alfredo Yang (:alfredo)]

(In reply to Sotaro Ikeda [:sotaro] from comment #54)
> 
> > I thought AtomicRefCountedWithFinalize is thread safe but it isn't. I'll
> > rewrite this part with lock.
> 
> Can you explain about why you thought it? We cannot use the lock here. It
> degrade performance and increase system resource usages.

Hmm...
I'm sorry that I don't explain it very well. I mean rewrite GonkTextureClientRecycleHandler with lock, not AtomicRefCountedWithFinalize.

Comment 60

[Alfredo Yang (:alfredo)]

(In reply to Sotaro Ikeda [:sotaro] from comment #48)
> (In reply to Sotaro Ikeda [:sotaro] from comment #47)
> > 
> > I understand, ResolveIfExists() operation is not atomic. Therefore
> > mPromise.RejectIfExists() and mPromise.ResolveIfExists() could cause race
> > condition.
> 
> One solution to it seems using Monitor and SetMonitor().
>   MozPromiseHolder::SetMonitor(Monitor* aMonitor)
>  
> https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/MozPromise.h#742

I'll try this way.

Comment 61

[Alfredo Yang (:alfredo)]

Attachment: video_omx_pdm

Comment 62

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8705131 [details] [diff] [review]
video_omx_pdm

nical, can you review gfx related part?

Comment 63

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8705131 [details] [diff] [review]
video_omx_pdm

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

Looks good!

::: dom/media/platforms/omx/OmxDataDecoder.cpp
@@ +345,5 @@
> +  //   proper abstracting it.
> +  // TODO:
> +  //   The buffer could be kept by layers after decoder is shutting down.
> +  //   The promise will keep all promise unsolved in CollectBufferPromises() if
> +  //   layer doesn't call recycle buffer.

You might want to update the comment. It seems not fit to current implementation.

Comment 64

[Alfredo Yang (:alfredo)]

(In reply to Sotaro Ikeda [:sotaro] from comment #63)
> Comment on attachment 8705131 [details] [diff] [review]
> video_omx_pdm
> 
> Review of attachment 8705131 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> ::: dom/media/platforms/omx/OmxDataDecoder.cpp
> @@ +345,5 @@
> > +  //   proper abstracting it.
> > +  // TODO:
> > +  //   The buffer could be kept by layers after decoder is shutting down.
> > +  //   The promise will keep all promise unsolved in CollectBufferPromises() if
> > +  //   layer doesn't call recycle buffer.
> 
> You might want to update the comment. It seems not fit to current
> implementation.

I'll update the comment.

Thank you for review! It does help a lot.

Comment 65

[Alfredo Yang (:alfredo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04e89aff1c60

Comment 66

[Nicolas Silva [:nical]]

Comment on attachment 8705131 [details] [diff] [review]
video_omx_pdm

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

::: dom/media/platforms/omx/GonkOmxPlatformLayer.cpp
@@ +213,5 @@
> +  }
> +
> +private:
> +  // Because TextureClient calls RecycleCallbackl when ref count is 1, so we
> +  // should hold only one reference here and use raw point when out of this class.

nit: raw pointer

Comment 67

[Alfredo Yang (:alfredo)]

Attachment: video_omx_pdm

Update comments.

Comment 68

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56062f372c4c

Comment 69

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/56062f372c4c