Implement OpenMax IL H264 video decoding client.

RESOLVED FIXED in Firefox 46

Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ayang, Assigned: ayang)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 9 obsolete attachments)

No description provided.
Depends on: 1224887
Priority: -- → P2
Posted patch video_omx_pdm (obsolete) — Splinter Review
This patch just works; however, it has to do some re-factory works because it is written under tiredness jet lag in WW.
Assignee: nobody → ayang
Posted patch video_omx_pdm (obsolete) — Splinter Review
A better draft.
Attachment #8697044 - Attachment is obsolete: true
Posted patch video_omx_pdm (obsolete) — Splinter Review
Attachment #8697588 - Attachment is obsolete: true
Depends on: 1231690, 1231257
Depends on: 1231939
Posted patch video_omx_pdm (obsolete) — Splinter Review
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.
Attachment #8699347 - Attachment is obsolete: true
Attachment #8699444 - Flags: review?(sotaro.ikeda.g)
Summary: Implement OpenMax IL video decoding client. → Implement OpenMax IL H264 video decoding client.
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.
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.
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.
Normally, input buffer size of video omx il with the video was 786432.
(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.
(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.
(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.
(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.
(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?
As you assumed, if I fixed as to use only sw video codec, the video playback failed with the crash.
Blocks: 1229363
Posted patch video_omx_pdm (obsolete) — Splinter Review
1. release resource at DoAsyncShutdown().
2. correct assertion.
Attachment #8699444 - Attachment is obsolete: true
Attachment #8699444 - Flags: review?(sotaro.ikeda.g)
Attachment #8701736 - Flags: review?(sotaro.ikeda.g)
Conformation of the patch seems to be blocked by Bug 1232348. It makes the video playback unstable.
Depends on: 1232348
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.
Attachment #8701736 - Flags: review?(sotaro.ikeda.g)
(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.
Depends on: 1235340
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)?
(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().
(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?
You might want to add the following handling.
  ro.moz.omx.hw.max_height
  ro.moz.omx.hw.max_width
(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" ?
(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.
(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 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.
(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.
Flags: needinfo?(sotaro.ikeda.g)
(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.
Posted patch video_omx_pdm (obsolete) — Splinter Review
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.
Attachment #8701736 - Attachment is obsolete: true
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8703520 - Flags: feedback?(sotaro.ikeda.g)
(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 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.
(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 )."
(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.
(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.
Flags: needinfo?(jolin)
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.
Attachment #8703520 - Flags: feedback?(sotaro.ikeda.g)
Posted patch temporary patch (obsolete) — Splinter Review
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.
(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].
:alfredo, how about attachment 8704089 [details] [diff] [review] way of implementation?
Flags: needinfo?(ayang)
(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.
Flags: needinfo?(ayang)
(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!
Flags: needinfo?(jolin)
Posted patch video_omx_pdm (obsolete) — Splinter Review
1. Update according to comment 17.
2. Use ITextureClientRecycleAllocator.
3. Update incorrect width/height.
Attachment #8703520 - Attachment is obsolete: true
Attachment #8704089 - Attachment is obsolete: true
Attachment #8704504 - Flags: review?(sotaro.ikeda.g)
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.
Attachment #8704504 - Flags: review?(sotaro.ikeda.g)
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?
(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.
(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 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.
(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.
In my understanding, the Monitor needs to be held by the caller during calling ResolveIfExists().
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.
(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.
(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.
Flags: needinfo?(ayang)
(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 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.
(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.
(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
Flags: needinfo?(ayang)
(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.
(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.
Posted patch video_omx_pdm (obsolete) — Splinter Review
Attachment #8704504 - Attachment is obsolete: true
Attachment #8705131 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8705131 [details] [diff] [review]
video_omx_pdm

nical, can you review gfx related part?
Attachment #8705131 - Flags: review?(nical.bugzilla)
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.
Attachment #8705131 - Flags: review?(sotaro.ikeda.g) → review+
(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 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
Attachment #8705131 - Flags: review?(nical.bugzilla) → review+
Posted patch video_omx_pdmSplinter Review
Update comments.
Attachment #8705131 - Attachment is obsolete: true
Attachment #8705641 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/56062f372c4c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.