Movie playback acceleration for Yocto based devices

RESOLVED FIXED in Firefox 61

Status

()

P3
enhancement
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: ashie, Assigned: ashie)

Tracking

(Blocks: 1 bug)

unspecified
mozilla61
ARM
Linux
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

2 years ago
Movie playback (especially H.264 & AAC) acceleration is required for Yocto based devices.

Although GStreamer is the de facto standard media framework in Yocto world, unfortunately GStreamer support of Gecko is already dropped (Bug 1234092). Some SoC vendors provides OpenMAX IL library for Yocto but we cannot expect that it's always available. For example, i.MX6 doesn't provide OpenMAX on Yocto.

Which way we have to choose?:

* Port to each SoC using it's native API
  * some (or most?) of them provides OpenMAX
* Revive or re-implement GStreamer support
(Assignee)

Comment 1

2 years ago
In bug1305341, we are now porting Gecko to following SoC:

* Renesas RZ/G1 series
    * Native codecs: provided as OpenMAX IL library
    * GStreamer: available via wrapper library (gst-omx)
* NXP i.MX6 series
    * Native codecs: original API (libvpu)
    * GStreamer: available

Since our current work is based on ESR45 which has GStreamer support, I'm now trying to enable acceleration via GStreamer. Although it can enable native codecs, I caught high CPU usage than software codecs. I'm now investigating this issue...
(Assignee)

Comment 2

2 years ago
On the other hand I'm also investigating the way to use OpenMAX. 

Although Gecko has OpenMAX support, it depends on Android. It seems that most codes are placed under "android" namespace. I'm not sure whether it can be easily port to Yocto (pure GNU/Linux) or not.
(Assignee)

Comment 3

2 years ago
> I caught high CPU usage than software codecs.

"software codecs" means FFmpeg.
(Assignee)

Comment 4

2 years ago
> I caught high CPU usage than software codecs. I'm now investigating this issue...

FYI:

  https://github.com/mozilla-japan/gecko-embedded/issues/3
  (Sorry, written in Japanese)
Gstreamer won't be reintroduced , it's fundamentally incompatible with HTML5 media source extension. 

HW acceleration enabled via gstreamer wouldn't provide much benefit as you would need to use basic layer (gstreamer copies everything back to a a software buffer)
(Assignee)

Comment 6

2 years ago
Thank you for your comment.

I have a question about OpenMAX. I'm now reading dom/media/platform/omx.
If I want to implement OpenMAX support, all I have to do is implementing child class of OmxPlatformLayer?
Alfredo - what is the current state of OMX support?
Flags: needinfo?(ayang)
(In reply to Takuro Ashie from comment #6)
> Thank you for your comment.
> 
> I have a question about OpenMAX. I'm now reading dom/media/platform/omx.
> If I want to implement OpenMAX support, all I have to do is implementing
> child class of OmxPlatformLayer?

You need to implement OmxPlatformLayer and OmxPromiseLayer::BufferData.

OmxPlatformLayer is a wrapper for OMX api, it should be straight forward to implement it.

BufferData could be little complicated depends on video buffer type.
If you plan to use graphic accelerated buffer, BufferData acts as a wrapper of graphic buffer. Otherwise, it just likes the normal OMX buffer.

You can refer to GonkOmxPlatformLayer, it should be able to provide you basic idea how to implement it.
Let me know it you have other questions.
Flags: needinfo?(ayang)
(Assignee)

Updated

2 years ago
See Also: → bug 1203859
(Assignee)

Comment 9

2 years ago
Thanks! I'll try to implement it.

Memo: In addition OmxDecoderModule have to be initiated at dom/media/platforms/PDMFactory.cpp
(Assignee)

Updated

2 years ago
See Also: → bug 1307238
(Assignee)

Updated

2 years ago
OS: All → Linux
Priority: -- → P3
(Assignee)

Comment 10

2 years ago
Created attachment 8805487 [details] [diff] [review]
Part1: Update the code base of dom/media/platform/omx in ESR45
(Assignee)

Comment 11

2 years ago
Created attachment 8805489 [details] [diff] [review]
Add the initial implementation of PureOmxPlatformLayer

It's a concrete class of OmxPlatformLayer for accessing OpenMAX IL libraries directly.
Although it's not stable yet, I confirmed that it works on Renesas's RZ/G and reduce CPU usage than FFmpeg.
(Assignee)

Comment 12

2 years ago
FYI: I'm now developing it at the following repository:

https://github.com/mozilla-japan/gecko-dev/tree/esr45-omx-linux
(Assignee)

Comment 13

2 years ago
Created attachment 8811109 [details] [diff] [review]
Part1: Update the code base of dom/media/platform/omx in ESR45 (v2)

Fix the wrong commit comment.
Attachment #8805487 - Attachment is obsolete: true
(Assignee)

Comment 14

2 years ago
Created attachment 8811111 [details] [diff] [review]
Part2: Add the initial implementation of PureOmxPlatformLayer (v2)

Improve stability:

* Use TaskQueue to dispatch callbakcs from OpenMAX
* Release OMX buffers correctly
Attachment #8805489 - Attachment is obsolete: true
(Assignee)

Comment 15

2 years ago
TODO:

* Port to mozilla-central
* Port to Raspberry Pi
* Implement zero-copy mode by using OMX_UseEGLImage()
(Assignee)

Updated

2 years ago
See Also: → bug 1210726
(Assignee)

Comment 16

2 years ago
Created attachment 8873305 [details] [diff] [review]
Part1: Add the initial implementation of PureOmxPlatformLayer to ESR52

I pick the latest patch from https://github.com/mozilla-japan/meta-browser/tree/firefox-52.1esr.
It's for ESR52, and I confirmed it only on Renesas's RZ/G1 series.
It seems that it can't be applied against current trunk. I'll fix it.
Attachment #8811109 - Attachment is obsolete: true
Attachment #8811111 - Attachment is obsolete: true
(Assignee)

Comment 17

2 years ago
Created attachment 8873307 [details] [diff] [review]
Part2: Fix a crash on opening about:support (for ESR52)
(Assignee)

Updated

2 years ago
Assignee: nobody → ashie
Status: NEW → ASSIGNED
Hi. Are you still interested in implementing this? We're considering removing the OMX support code, since it's currently unused.
Flags: needinfo?(ashie)
(Assignee)

Comment 19

9 months ago
Thank you for your notification.
Yes, we are still interesting in it.
Just now we are porting the code to m-c.

https://github.com/KSR-Yasuda/meta-browser/tree/feature/firefox-60/OpenMAX/recipes-mozilla/firefox/firefox/openmax
https://github.com/webdino/gecko-dev/tree/omx-linux-dev-wip

It seems that the current code can decode a first frame of a H.264 movie but cannot decode following frames (on Renesas RZ/G1 series). We are now investigating this issue.

Since the Wayland port is recently landed on m-c (bug635134), we are now catching up m-c (our main target is Wayland).
Flags: needinfo?(ashie)
Ok, thanks for the update. Dan, it looks like we have downstream users of OMX, so we should keep the code as long as they're willing to help maintain it.
(Assignee)

Comment 22

9 months ago
(In reply to Takuro Ashie from comment #19)
> It seems that the current code can decode a first frame of a H.264 movie but
> cannot decode following frames (on Renesas RZ/G1 series). We are now

It's not correct.
It can play when I specify a movie file directly.

e.g.) $ firefox https://www.quirksmode.org/html5/videos/big_buck_bunny.mp4

But it can't play anymore when I press the play button after the first playback is finished.
It seems that the decoder thread is stalled on shutting down.

(When a HTML file with a video element is loaded, it will be stalled after decoding the preview picture.
This is the reason I thought that it can decode only one frame.)

The issue seems resolved by the following patch:

* https://github.com/webdino/gecko-dev/commit/c8cd0ca2e914a2bd4c7f7fc15d49feffd6c1ae41

I'll attach it to this bug after I clean up all patches.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8873305 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8873307 - Attachment is obsolete: true

Comment 25

8 months ago
mozreview-review
Comment on attachment 8962637 [details]
Bug 1306529 - Add initial implementation of PureOmxPlatformLayer

https://reviewboard.mozilla.org/r/231492/#review236980

This is great... Thank you for doing this...

some minor changes required.
r- not because the quality of the code isn't there, but because I just want to have a look at it again.

::: dom/media/MediaPrefs.h:128
(Diff revision 1)
>    DECL_MEDIA_PREF("media.navigator.hardware.vp8_encode.acceleration_remote_enabled", RemoteMediaCodecVP8EncoderEnabled, bool, false);
>  #endif
>    // WebRTC
>    DECL_MEDIA_PREF("media.navigator.mediadatadecoder_enabled", MediaDataDecoderEnabled, bool, false);
> +#ifdef MOZ_WIDGET_GTK
> +  DECL_MEDIA_PREF("media.pdm-omx.enabled",                    PDMOmxEnabled, bool, false);

please use a different preference, more in line with the others.
media.omx.enabled

::: dom/media/platforms/PDMFactory.cpp:25
(Diff revision 1)
>  #include "AppleDecoderModule.h"
>  #endif
>  #ifdef MOZ_WIDGET_ANDROID
>  #include "AndroidDecoderModule.h"
>  #endif
> +#ifdef MOZ_WIDGET_GTK

this doesn't appear to me to be the right config to use.

GTK presence has little to do with OMX, and will enable the code on all linux machine.

Please add a new build option specifically for embedded linux platform

::: dom/media/platforms/moz.build:75
(Diff revision 1)
>      ]
>      UNIFIED_SOURCES += [
>          'agnostic/AOMDecoder.cpp',
>      ]
>  
> +if toolkit == 'gtk3':

same deal... gtk3 doesn't appear to be the right choice here and is using a config not related to OMX.

Create a new one so this won't be compiled/enabled for plain linux build

::: dom/media/platforms/omx/OmxDataDecoder.h:164
(Diff revision 1)
>    CollectBufferPromises(OMX_DIRTYPE aType);
>  
>    // The Omx TaskQueue.
>    RefPtr<TaskQueue> mOmxTaskQueue;
>  
> +  RefPtr<TaskQueue> mReaderTaskQueue;

this isn't the reader's taskqueue, but the MediaDataDecoder task queue, just call it mTaskQueue otherwise it's confusing...

The reader runs on a different taskqueue than this one

::: dom/media/platforms/omx/OmxDecoderModule.cpp:31
(Diff revision 1)
> +
> +OmxDecoderModule*
> +OmxDecoderModule::Create()
> +{
> +#ifdef MOZ_WIDGET_GTK
> +  if (!Init())

use braces for any if statement, even single line one:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Naming_and_Formatting_code

::: dom/media/platforms/omx/OmxDecoderModule.cpp:33
(Diff revision 1)
> +OmxDecoderModule::Create()
> +{
> +#ifdef MOZ_WIDGET_GTK
> +  if (!Init())
> +    return nullptr;
> +  return new OmxDecoderModule();

if (Init()) {
  return new OmxDecoderModule();
}

then you don't need two times return nullptr.

::: dom/media/platforms/omx/OmxPlatformLayer.cpp:305
(Diff revision 1)
> +OmxPlatformLayer::Create(OmxDataDecoder* aDataDecoder,
> +                         OmxPromiseLayer* aPromiseLayer,
> +                         TaskQueue* aTaskQueue,
> +                         layers::ImageContainer* aImageContainer)
> +{
> +  return new PureOmxPlatformLayer(aDataDecoder, aPromiseLayer, aTaskQueue, aImageContainer);

80 columns formatting.

::: modules/libpref/init/all.js:419
(Diff revision 1)
>  pref("media.wmf.deblacklisting-for-telemetry-in-gpu-process", true);
>  pref("media.wmf.play-stand-alone", true);
>  pref("media.wmf.use-sync-texture", true);
>  #endif
> +#if defined(MOZ_WIDGET_GTK)
> +pref("media.pdm-omx.enabled", false);

media.omx.enabled
Attachment #8962637 - Flags: review-

Comment 26

8 months ago
mozreview-review
Comment on attachment 8962638 [details]
Bug 1306529 - OmxDataDecoder: Fix a stall issue on shutting down

https://reviewboard.mozilla.org/r/231494/#review236984

::: dom/media/platforms/omx/OmxDataDecoder.cpp:307
(Diff revision 1)
> +              [self] () {
> +                // Since all tasks are already done, it will return immediately.
> -             self->mOmxTaskQueue->BeginShutdown();
> +                self->mOmxTaskQueue->BeginShutdown();
> -             self->mOmxTaskQueue->AwaitShutdownAndIdle();
> +                self->mOmxTaskQueue->AwaitShutdownAndIdle();
> -             self->mShutdownPromise.Resolve(true, __func__);
> +                self->mShutdownPromise.Resolve(true, __func__);
> +                return ShutdownPromise::CreateAndResolve(true, __func__);

good catch... Yes, AwaisShutdownAndIdle would block forever if run on the mOmxTaskQueue.

this serves no purpose as your function is defined as void.

there's no need to return anything.

Also, don't use InvokeAsync, just dispatch a task instead on the mTaskQueue (introduced in patch #1) where the mShutdownPromise will be resolved.

You could have done the same in a more elegant manner by chaining the Then instead.
e.g.

->Then(mOmxTaskQueue, [self]() { ... })
->Then(mReaderTaskQueue), [self]() {
    self->mOmxTaskQueue->BeginShutdown();
    self->mOmxTaskQueue->AwaitShutdownAndIdle();
    self->mShutdownPromise.Resolve(true, __func__);
});

that way, no need for FinalizeAsyncShutown
Attachment #8962638 - Flags: review+
(Assignee)

Comment 27

8 months ago
mozreview-review
Comment on attachment 8962637 [details]
Bug 1306529 - Add initial implementation of PureOmxPlatformLayer

https://reviewboard.mozilla.org/r/231492/#review237440

::: dom/media/platforms/moz.build:81
(Diff revision 1)
> +    EXPORTS += [
> +        'omx/OmxCoreLibLinker.h',
> +    ]
> +    UNIFIED_SOURCES += [
> +        'omx/OmxCoreLibLinker.cpp',
> +    ]

Oops, I forgot to add OmxCoreLibLinker.{cpp,h}.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 30

8 months ago
mozreview-review-reply
Comment on attachment 8962637 [details]
Bug 1306529 - Add initial implementation of PureOmxPlatformLayer

https://reviewboard.mozilla.org/r/231492/#review237440

> Oops, I forgot to add OmxCoreLibLinker.{cpp,h}.

Sorry, some new files are missing in the previous patch.
I've added them.
(Assignee)

Comment 31

8 months ago
mozreview-review-reply
Comment on attachment 8962637 [details]
Bug 1306529 - Add initial implementation of PureOmxPlatformLayer

https://reviewboard.mozilla.org/r/231492/#review236980

> please use a different preference, more in line with the others.
> media.omx.enabled

I renamed it to media.omx.enabled.

> this doesn't appear to me to be the right config to use.
> 
> GTK presence has little to do with OMX, and will enable the code on all linux machine.
> 
> Please add a new build option specifically for embedded linux platform

> Please add a new build option specifically for embedded linux platform

I thinks it's hard to detect it because "embedded linux" is not so different from desktop linux.
Instead I'll add a simple enabled/disable switch (like `--enable-omx`).
Since OpenMAX isn't depend on Linux, it might be usable also on other OSs.

> this isn't the reader's taskqueue, but the MediaDataDecoder task queue, just call it mTaskQueue otherwise it's confusing...
> 
> The reader runs on a different taskqueue than this one

I renamed it to mTaskQueue.

> use braces for any if statement, even single line one:
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Naming_and_Formatting_code

Addressed.

> if (Init()) {
>   return new OmxDecoderModule();
> }
> 
> then you don't need two times return nullptr.

Addressed.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

8 months ago
mozreview-review-reply
Comment on attachment 8962637 [details]
Bug 1306529 - Add initial implementation of PureOmxPlatformLayer

https://reviewboard.mozilla.org/r/231492/#review236980

> > Please add a new build option specifically for embedded linux platform
> 
> I thinks it's hard to detect it because "embedded linux" is not so different from desktop linux.
> Instead I'll add a simple enabled/disable switch (like `--enable-omx`).
> Since OpenMAX isn't depend on Linux, it might be usable also on other OSs.

I added `--enable-openmax` option.
The default value is disabled.

Comment 35

8 months ago
mozreview-review
Comment on attachment 8962637 [details]
Bug 1306529 - Add initial implementation of PureOmxPlatformLayer

https://reviewboard.mozilla.org/r/231492/#review237850

::: dom/media/platforms/omx/OmxCoreLibLinker.h:29
(Diff revision 3)
> +  static const char* sLibNames[];
> +
> +  static bool TryLinkingLibrary(const char *libName);
> +  static bool Bind(const char* aLibName);
> +
> +  static enum LinkStatus {

bracket on next line

::: dom/media/platforms/omx/OmxCoreLibLinker.cpp:53
(Diff revision 3)
> +      sLibName = libName;
> +      sLinkStatus = LinkStatus_SUCCEEDED;
> +      LOG("Succeeded to load %s", libName);
> +      return true;
> +    } else {
> +      LOG("Failed to link %s", libName);

shouldnt you ser sLinkStatus to fail here? otherwise it will keep retrying over and over

::: dom/media/platforms/omx/PureOmxPlatformLayer.h:16
(Diff revision 3)
> +
> +namespace mozilla {
> +
> +class PureOmxPlatformLayer;
> +
> +class PureOmxBufferData : public OmxPromiseLayer::BufferData {

bracket on new line, per style

::: dom/media/platforms/omx/PureOmxPlatformLayer.h:33
(Diff revision 3)
> +  bool ShouldUseEGLImage();
> +
> +  const PureOmxPlatformLayer& mPlatformLayer;
> +  const OMX_PARAM_PORTDEFINITIONTYPE mPortDef;
> +};
> +

bracket

::: dom/media/platforms/omx/PureOmxPlatformLayer.h:47
(Diff revision 3)
> +                       TaskQueue* aTaskQueue,
> +                       layers::ImageContainer* aImageContainer);
> +
> +  virtual ~PureOmxPlatformLayer();
> +
> +  virtual OMX_ERRORTYPE InitOmxToStateLoaded(const TrackInfo* aInfo) override;

virtual and override together are redundant.
an override is always virtual.

plesse remove virtual keyword

::: dom/media/platforms/omx/PureOmxPlatformLayer.cpp:226
(Diff revision 3)
> +
> +    // Although the ref count of the buffer may not be 0 at this moment, we need
> +    // to force release all OpenMAX buffers to flush OMX_CommandStateSet for
> +    // switching the state to OMX_StateLoaded.
> +    // See OmxDataDecoder::DoAsyncShutdown() for more detail.
> +    buffer->ReleaseBuffer();

that sounds scary.
ifnthe refcount is not 0, it indicates that the buffer is still in use somewhere... which if now freed could be easily lesd to a UAF.

and rather than manually having to call ReleaseBuffer, cant we have a wrapper class, like RefPtr that will handle the release automatically?

::: dom/media/platforms/omx/PureOmxPlatformLayer.cpp:400
(Diff revision 3)
> +
> +  OMX_U32 nComponents = 0;
> +  OMX_ERRORTYPE err;
> +  err = OMX_GetComponentsOfRole(const_cast<OMX_STRING>(role.Data()),
> +                                &nComponents, nullptr);
> +  if (err != OMX_ErrorNone || nComponents <= 0)

brackets missing

::: dom/media/platforms/omx/PureOmxPlatformLayer.cpp:409
(Diff revision 3)
> +
> +  // TODO:
> +  // Only the first component will be used.
> +  // We should detect the most preferred component.
> +  OMX_U8* componentNames[1];
> +  componentNames[0] = reinterpret_cast<OMX_U8*>(malloc(OMX_MAX_STRINGNAME_SIZE));

please using UniquePtr along new[] rather than malloc

::: dom/media/platforms/omx/PureOmxPlatformLayer.cpp:431
(Diff revision 3)
> +{
> +  nsAutoCString componentName;
> +  if (aComponentName) {
> +    componentName = *aComponentName;
> +  } else {
> +    bool found = FindStandardComponent(mInfo->mMimeType, &componentName);

temporary found is not necessary here

::: dom/media/platforms/omx/PureOmxPlatformLayer.cpp:432
(Diff revision 3)
> +  nsAutoCString componentName;
> +  if (aComponentName) {
> +    componentName = *aComponentName;
> +  } else {
> +    bool found = FindStandardComponent(mInfo->mMimeType, &componentName);
> +    if (!found)

missing brackets.

::: modules/libpref/init/all.js:412
(Diff revision 3)
>  pref("media.wmf.deblacklisting-for-telemetry-in-gpu-process", true);
>  pref("media.wmf.play-stand-alone", true);
>  pref("media.wmf.use-sync-texture", true);
>  #endif
> +#if defined(MOZ_OMX)
> +pref("media.omx.enabled", true);

i thought that the pref was disabled by default (per commit message).
either update the commit message, or changr the pref default

::: toolkit/moz.configure:404
(Diff revision 3)
>  add_old_configure_assignment('MOZ_FMP4', fmp4)
>  
> +# OpenMAX IL Decoding Support
> +# ==============================================================
> +option('--enable-openmax',
> +       help='Enable OpenMAX IL for fragmented video/audio decoding')

for videp/audio decoding
fragmented should be removed
Attachment #8962637 - Flags: review-

Comment 36

8 months ago
mozreview-review
Comment on attachment 8962638 [details]
Bug 1306529 - OmxDataDecoder: Fix a stall issue on shutting down

https://reviewboard.mozilla.org/r/231494/#review237858

::: dom/media/platforms/omx/OmxDataDecoder.cpp:290
(Diff revision 3)
> -           [self] () {
> +           [self] () -> RefPtr<ShutdownPromise> {
>               self->mOmxLayer->Shutdown();
>               self->mWatchManager.Shutdown();
>               self->mOmxLayer = nullptr;
>               self->mMediaDataHelper = nullptr;
> -
> +             return ShutdownPromise::CreateAndReject(false, __func__);

a shutdown promise must always be resolved....

we have strong assert that thks is the case upstream in the code.

so only ever resolve the ShutdownPromise

there is nothing more we can do anyway.

then in the Then reject case, you can simply have MOZ_CRASH("can't be reached") or something like that
Attachment #8962638 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 39

8 months ago
mozreview-review-reply
Comment on attachment 8962637 [details]
Bug 1306529 - Add initial implementation of PureOmxPlatformLayer

https://reviewboard.mozilla.org/r/231492/#review237850

> shouldnt you ser sLinkStatus to fail here? otherwise it will keep retrying over and over

Oops. Yes, LinkStatus_FAILED should be set after all candidates are failed.
I moved setting sLinkStatus to OmxCoreLibLinker::Link() to do it.

> please using UniquePtr along new[] rather than malloc

I replaced it with UniquePtr & MakeUniqueFallible().

> i thought that the pref was disabled by default (per commit message).
> either update the commit message, or changr the pref default

I've updated the commit message.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 42

8 months ago
mozreview-review-reply
Comment on attachment 8962637 [details]
Bug 1306529 - Add initial implementation of PureOmxPlatformLayer

https://reviewboard.mozilla.org/r/231492/#review237850

> that sounds scary.
> ifnthe refcount is not 0, it indicates that the buffer is still in use somewhere... which if now freed could be easily lesd to a UAF.
> 
> and rather than manually having to call ReleaseBuffer, cant we have a wrapper class, like RefPtr that will handle the release automatically?

Sorry, the comment was inappropriate.
Releasing *raw* OpenMAX buffers here is required by OmxDataDecoder.
The function `ReleaseOmxBuffer()` is the function for the purpose.
It's the design of `OmxDataDecoder` and `OmxPlatformLayer`.

Here is the comment in `OmxDataDecoder::DoAsyncShutdown()` which describes the reason:

>             // According to spec 3.1.1.2.2.1:                                                                                                    
>             // OMX_StateLoaded needs to be sent before releasing buffers.                                                                        
>             // And state transition from OMX_StateIdle to OMX_StateLoaded                                                                        
>             // is completed when all of the buffers have been removed                                                                            
>             // from the component.                                                                                                               
>             // Here the buffer promises are not resolved due to displaying                                                                       
>             // in layer, it needs to wait before the layer returns the                                                                           
>             // buffers.                                                                                                                          

The refcounts of the wrapper objects `PureOmxBufferData` (a child class of OmxPromiseLayer::BufferData) are decreased to `0` safely after that and released automatically.

I removed the ambiguous description in the comment.
(Assignee)

Comment 43

8 months ago
mozreview-review-reply
Comment on attachment 8962638 [details]
Bug 1306529 - OmxDataDecoder: Fix a stall issue on shutting down

https://reviewboard.mozilla.org/r/231494/#review237858

> a shutdown promise must always be resolved....
> 
> we have strong assert that thks is the case upstream in the code.
> 
> so only ever resolve the ShutdownPromise
> 
> there is nothing more we can do anyway.
> 
> then in the Then reject case, you can simply have MOZ_CRASH("can't be reached") or something like that

I simply modified it to Resolve().
It's the equivalent to the previous code.
And the previous code ensure to shutdown safely even in this case.
(Assignee)

Updated

7 months ago
Attachment #8962637 - Flags: review?(jyavenard)
(Assignee)

Updated

7 months ago
Attachment #8962637 - Flags: review?(jyavenard)

Comment 44

7 months ago
mozreview-review
Comment on attachment 8962637 [details]
Bug 1306529 - Add initial implementation of PureOmxPlatformLayer

https://reviewboard.mozilla.org/r/231492/#review240486
Attachment #8962637 - Flags: review?(jyavenard) → review+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 45

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7e70a7f597c4
Add initial implementation of PureOmxPlatformLayer r=jya
https://hg.mozilla.org/integration/autoland/rev/87206102c699
OmxDataDecoder: Fix a stall issue on shutting down r=jya
Keywords: checkin-needed
Please use the try feature next time....
(Assignee)

Comment 48

7 months ago
Sorry, an unnecessary dependency "target" in remained in the configure option.

> +@depends('--enable-openmax', target)
> +def openmax(value, target):
> +    enabled = bool(value)
> +    if enabled:
> +        return True

I'll remove it.


> Please use the try feature next time....

I don't have access to tryserver.
So I've filed bug1452848 to request it.
Could you vouch me, please?
Flags: needinfo?(ashie) → needinfo?(jyavenard)
Flags: needinfo?(jyavenard)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8962637 - Flags: review+ → review?(jyavenard)
(Assignee)

Updated

7 months ago
Attachment #8962638 - Flags: review+ → review?(jyavenard)
(Assignee)

Comment 51

7 months ago
> Sorry, an unnecessary dependency "target" in remained in the configure option.
> 
> > +@depends('--enable-openmax', target)
> > +def openmax(value, target):
> > +    enabled = bool(value)
> > +    if enabled:
> > +        return True
> 
> I'll remove it.

Done.

Running test on try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58aeaf434a6bf3874460e4d2ae70b5dc377100ad
Comment on attachment 8962637 [details]
Bug 1306529 - Add initial implementation of PureOmxPlatformLayer

I've already r+ those changes, you don't need to ask again when it's just about implementing the changes requested earlier.

thank you for this great work once again
Attachment #8962637 - Flags: review?(jyavenard)
Attachment #8962638 - Flags: review?(jyavenard)

Comment 53

7 months ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8cbd855ad7c5462132d1ee8f5910cb39057102ac -d 01b20ae8a91b: rebasing 458809:8cbd855ad7c5 "Bug 1306529 - Add initial implementation of PureOmxPlatformLayer r=jya"
other [source] changed dom/media/MediaPrefs.h which local [dest] deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
merging dom/media/platforms/PDMFactory.cpp
merging modules/libpref/init/all.js
merging toolkit/moz.configure
warning: conflicts while merging modules/libpref/init/all.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Summary: Movie playback acceletion for Yocto based devices → Movie playback acceleration for Yocto based devices
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 56

7 months ago
> I've already r+ those changes, you don't need to ask again when it's just about implementing the changes requested earlier.

I see, thanks!

But...sorry, I've added a little modification again to resolve the conflict.
It follows the change of MediaPrefs (bug1448222).
Could you review it just in case?
Sorry to bother you again.

Test on tryserver:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c3d18513063584028a831132da662b436f5f835
Flags: needinfo?(jyavenard)

Comment 57

7 months ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7db89b0787b
Add initial implementation of PureOmxPlatformLayer r=jya
https://hg.mozilla.org/integration/autoland/rev/d81ab6f099ee
OmxDataDecoder: Fix a stall issue on shutting down r=jya

Comment 58

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7db89b0787b
https://hg.mozilla.org/mozilla-central/rev/d81ab6f099ee
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: needinfo?(jyavenard)
Duplicate of this bug: 1203859
You need to log in before you can comment on or make changes to this bug.