The default bug view has changed. See this FAQ.

support for hardware-accelerated audio/video decoding (part 1: libstagefright/OMX support for Gonk)

RESOLVED FIXED in mozilla15

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: gal, Assigned: cajbir)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

(Whiteboard: [qa+][caf:helping], URL)

Attachments

(5 attachments, 41 obsolete attachments)

65.57 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
174.54 KB, patch
Details | Diff | Splinter Review
2.14 KB, patch
eflores
: review+
Details | Diff | Splinter Review
5.66 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
40.71 KB, image/png
Details
(Reporter)

Description

5 years ago
This is the beginnings of a patch to add audio/video decoding via libstagefright/OMX to Gecko. This likely will only ever work with Gonk where we have solid control of the libstagefright source and OEM-portions of OMX, due to Android's non-existing clean APIs for this.

How does libstagefright work?

libstagefright implements a bunch of data sources, including a HTTP streaming client (DataSource). DataSource can be passed to a MediaExtractor, which sniffs for the media type and splits it into one or two MediaSource streams (depending on whether we have audio or video or both). MediaSource is piped into the actual decoders, which are controlled through OMXCodec, which contains a lot of horrible quirks to deal with OEMs being unable to write codecs in any sane way. The decoders spit out MediaBuffers, which are basically a video frame or a chunk of audio data. MetaData rides along and can be used to figure out the format. There are two ways to render MediaBuffers. For some MediaBuffers the data is available in them and can be read out in one of a few YUV formats. Others (in particular hardware decoders) don't cough up the data. Instead, the MediaBuffer contains a bufferId which can be passed to a OEM-specific renderer which knows how to put it onto an overlay. The latter allows secure decoders where the image data is not actually accessible at all.

How does this work with Gecko?

We are not using libstagefright's clownshoes of a network/buffering layer. Instead, we wrap nsMediaStream into a DataSource and hand that to the decoder. OMXGlue.cpp contains code to invoke the decoder, and it also contains declarations the decoder-side uses to access the wrapped nsMediaStream. When decoding audio and video we have 2 frames in flight at all times so we can determine the current time as well as the end time of a frame (which is the current time of the next frame). The YUV data of libstagefright can be planar (which we already support), as well as semi-planar (y and uv). I added extra fields to VideoData to express that. We should rename YUV_PLANAR to just YUV and add support to ImageLayer and change the shaders accordingly. I will do that in the next version of the patch.
(Reporter)

Comment 1

5 years ago
Created attachment 585104 [details] [diff] [review]
patch
Assignee: nobody → gal
(Reporter)

Comment 2

5 years ago
I forgot to mention what we are going to do with MediaBuffers that don't allow the YUV content to be extracted. I will wire up the binary blob that does the rendering to render that content directly to the screen using an overlay, or if I can trick it to render into video memory, we will have it render into a texture. We will wrap this magical kind of image into a new kind of ImageLayer, similar as we do for certain textures on Mac.
(Reporter)

Comment 3

5 years ago
I think I may have this almost working on Android. Its quite a hack and it will require special handling for each major version of android (2.2 2.3 4.0 etc), but I think it can be done OEM-neutral, which would make this feasible for product deployment. I will poke at this some more and post a more detailed summary.
(Reporter)

Comment 4

5 years ago
Created attachment 585734 [details] [diff] [review]
patch

Factor code that talks to libstagefright into libomxplugin.so and load it dynamically through a clean interface. Build dummy libutils/libbinder/libstagefright/libmediaplayerservice and link the plugin against that. At load time the real libs will be picked up. If that fails, firefox still loads (hence the libomxplugin). MPAPI.h defines how libomxplugin.so talks to the media stack. I will probably factor overlay support into the same API. This is still not working right, this is just a code dump.
Attachment #585104 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #585734 - Attachment is patch: true
(Reporter)

Comment 5

5 years ago
(Note: the android source has to be present to build libomxplugin since we use header files that are not in the NDK)
Blocks: 715784
(Reporter)

Comment 6

5 years ago
Making good progress here, new update soon.
(Reporter)

Comment 7

5 years ago
Created attachment 586367 [details] [diff] [review]
patch
Attachment #585734 - Attachment is obsolete: true
(Reporter)

Comment 8

5 years ago
Created attachment 588313 [details] [diff] [review]
patch
Attachment #586367 - Attachment is obsolete: true

Comment 9

5 years ago
Created attachment 592577 [details] [diff] [review]
make patch to b2g's source code

Comment 10

5 years ago
I created attachment 592577 [details] [diff] [review] patch.
it adds libstagefright/OMX-based decoding by using sw decoder and hw decoder. 

but still has problems, like folowing.
- drawed color of SemiPlanarYUV420Frame is not correct.
   + PlanarYUV420Frame seems correct
   + this might be a problem of ImageLayer
- played audio is not correct (weird sound)
- occasionally, b2g process crashes

Comment 11

5 years ago
592577 tentatively disables security chekeck of nsHTMLMediaElement.cpp
for operation cheking, I added video tag to a b2g' application. but It seems that httpd.js could not handle read request to Movie content. 

I do not know how to solve this problem. Then, simply use following file uri. To do this, the security check is temporarily disabled.

for program operation cheking, I use following code.
>  <video controls autoplay>
>  <source src="file:///system/home/apps/browser/sample.m4v" type="video/mp4"/>
>  </video>

Comment 12

5 years ago
Created attachment 592995 [details] [diff] [review]
patch to b2g (MediaPlayerService disabled)

topic of the patch
 - libstagefright/OMX-based decoding within b2g' process
 - MediaPlayerService disabled

It seems that the patch still has bugs to MediaBuffer handling.
If audio data handling is enabled, b2g process crashes, then it tentatively disables audio.
(Reporter)

Comment 13

5 years ago
Created attachment 596301 [details] [diff] [review]
Part 1: gonk patch to fix some quirks and disable old drivers for akami
Attachment #588313 - Attachment is obsolete: true
Attachment #592577 - Attachment is obsolete: true
Attachment #592995 - Attachment is obsolete: true
(Reporter)

Comment 14

5 years ago
Comment on attachment 596301 [details] [diff] [review]
Part 1: gonk patch to fix some quirks and disable old drivers for akami

>diff --git a/media/libstagefright/OMXCodec.cpp b/media/libstagefright/OMXCodec.cpp
>index b5d00bf..a0c81cc 100644
>--- a/media/libstagefright/OMXCodec.cpp
>+++ b/media/libstagefright/OMXCodec.cpp
>@@ -166,18 +166,18 @@ static const CodecInfo kDecoderInfo[] = {
> //    { MEDIA_MIMETYPE_AUDIO_AAC, "OMX.PV.aacdec" },
>     { MEDIA_MIMETYPE_AUDIO_G711_ALAW, "G711Decoder" },
>     { MEDIA_MIMETYPE_AUDIO_G711_MLAW, "G711Decoder" },
>-    { MEDIA_MIMETYPE_VIDEO_MPEG4, "OMX.qcom.7x30.video.decoder.mpeg4" },
>+//    { MEDIA_MIMETYPE_VIDEO_MPEG4, "OMX.qcom.7x30.video.decoder.mpeg4" },
>     { MEDIA_MIMETYPE_VIDEO_MPEG4, "OMX.qcom.video.decoder.mpeg4" },
>     { MEDIA_MIMETYPE_VIDEO_MPEG4, "OMX.TI.Video.Decoder" },
>     { MEDIA_MIMETYPE_VIDEO_MPEG4, "OMX.SEC.MPEG4.Decoder" },
>     { MEDIA_MIMETYPE_VIDEO_MPEG4, "M4vH263Decoder" },
> //    { MEDIA_MIMETYPE_VIDEO_MPEG4, "OMX.PV.mpeg4dec" },
>-    { MEDIA_MIMETYPE_VIDEO_H263, "OMX.qcom.7x30.video.decoder.h263" },
>+//    { MEDIA_MIMETYPE_VIDEO_H263, "OMX.qcom.7x30.video.decoder.h263" },
>     { MEDIA_MIMETYPE_VIDEO_H263, "OMX.qcom.video.decoder.h263" },
>     { MEDIA_MIMETYPE_VIDEO_H263, "OMX.SEC.H263.Decoder" },
>     { MEDIA_MIMETYPE_VIDEO_H263, "M4vH263Decoder" },
> //    { MEDIA_MIMETYPE_VIDEO_H263, "OMX.PV.h263dec" },
>-    { MEDIA_MIMETYPE_VIDEO_AVC, "OMX.qcom.7x30.video.decoder.avc" },
>+//    { MEDIA_MIMETYPE_VIDEO_AVC, "OMX.qcom.7x30.video.decoder.avc" },
>     { MEDIA_MIMETYPE_VIDEO_AVC, "OMX.qcom.video.decoder.avc" },
>     { MEDIA_MIMETYPE_VIDEO_AVC, "OMX.TI.Video.Decoder" },
>     { MEDIA_MIMETYPE_VIDEO_AVC, "OMX.SEC.AVC.Decoder" },
>@@ -381,6 +381,7 @@ uint32_t OMXCodec::getComponentQuirks(
>     if (!strncmp(componentName, "OMX.qcom.7x30.video.encoder.", 28)) {
>     }
>     if (!strncmp(componentName, "OMX.qcom.video.decoder.", 23)) {
>+        quirks |= kRequiresAllocateBufferOnInputPorts;
>         quirks |= kRequiresAllocateBufferOnOutputPorts;
>         quirks |= kDefersOutputBufferAllocation;
>     }
>@@ -3601,6 +3602,8 @@ void OMXCodec::initOutputFormat(const sp<MetaData> &inputFormat) {
>             } else {
>                 mOutputFormat->setInt32(kKeyWidth, video_def->nFrameWidth);
>                 mOutputFormat->setInt32(kKeyHeight, video_def->nFrameHeight);
>+                mOutputFormat->setInt32(kKeyStride, video_def->nStride);
>+                mOutputFormat->setInt32(kKeySliceHeight, video_def->nSliceHeight);
>             }
> 
>             mOutputFormat->setInt32(kKeyColorFormat, video_def->eColorFormat);
>diff --git a/media/mediaserver/main_mediaserver.cpp b/media/mediaserver/main_mediaserver.cpp
>index 7094cfa..1c9e020 100644
>--- a/media/mediaserver/main_mediaserver.cpp
>+++ b/media/mediaserver/main_mediaserver.cpp
>@@ -39,7 +39,7 @@ int main(int argc, char** argv)
>     sp<IServiceManager> sm = defaultServiceManager();
>     LOGI("ServiceManager: %p", sm.get());
>     AudioFlinger::instantiate();
>-    MediaPlayerService::instantiate();
>+    /* MediaPlayerService::instantiate(); */
>     CameraService::instantiate();
>     AudioPolicyService::instantiate();
>     ProcessState::self()->startThreadPool();
Attachment #596301 - Attachment is patch: true
(Reporter)

Comment 15

5 years ago
Created attachment 596302 [details] [diff] [review]
Part 2: MPAPI and omx-plugin for B2G
(Reporter)

Comment 16

5 years ago
Created attachment 596303 [details] [diff] [review]
Part 3: enable omx-plugin in build config
(Reporter)

Comment 17

5 years ago
Created attachment 596304 [details]
test case for B2G, install as /data/local/homescreen.html along with sample.mp4
(Reporter)

Updated

5 years ago
(Reporter)

Comment 18

5 years ago
The patch now avoids having more than one MediaBuffer outstanding, reducing the amount of patching necessary to libstagefright.

The attached patch plays the Big Buck Bunny H264 360p video with full framerate (30FPS) on akami.

Major issues:
- We are seeing horrific tearing on akami. The background shines through for every second frame or so. Looks like we aren't getting double buffering on the GL context on akami, and we pretty reliably get hit by vsync at the worst possible time. This is probably not the fault of this patch.
- On akami, B2G crashes after a few seconds or minutes. Its pretty easy to reproduce so shouldn't be too hard to debug.

An anonymous silicon vendor engineer contributed to this patch (in addition to Ikeda.)

Comment 19

5 years ago
Created attachment 596574 [details] [diff] [review]
informational patch

Patches I made were alrealy oblsolete.
then, this is just informational.
The patches had defects related to MediaBuffers hadling.
When Audio is enabled, B2G process crashes soon.
An attached patch fixes the defects.

By this fixes, "STEREO Audio" could be palyed.
But still has defect to "MONO audio handling".

Updated

5 years ago
Attachment #596574 - Attachment is obsolete: true

Updated

5 years ago
Attachment #596574 - Attachment is obsolete: false
Attachment #596574 - Attachment is patch: true

Updated

5 years ago
Attachment #596574 - Attachment is obsolete: true
(Reporter)

Comment 20

5 years ago
Ikeda, I forgot to attach my omx patch. I will do that in a sec. I removed using 2 buffers in Read() so now its not necessary any more to modify stagefright. Can you attach an interdiff so I see what you changed for the audio fix?
(Reporter)

Comment 21

5 years ago
Created attachment 596932 [details] [diff] [review]
part 2: gecko patch
Attachment #596302 - Attachment is obsolete: true
Attachment #596303 - Attachment is obsolete: true

Comment 22

5 years ago
(In reply to Andreas Gal :gal from comment #21)
> Created attachment 596932 [details] [diff] [review]
> part 2: gecko patch

Andreas, current "part 2: gecko patch" do not include follwoing files.
 - /B2G/gecko/content/media/plugins/*

Can you update the patch?
(Reporter)

Comment 23

5 years ago
Strange. Sure, let me check again.
(Reporter)

Comment 24

5 years ago
Created attachment 596938 [details] [diff] [review]
part 2: gecko patch
Attachment #596932 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #596938 - Attachment is patch: true
(Reporter)

Comment 25

5 years ago
Ok, let me know if this is complete. This still
- stops playback or crashes occasionally playing a long video
- has audio/video sync issues on seek (mostly video just stops)

Comment 26

5 years ago
Thank you for updating the patch. New patch is complete.

There seems following problems, in emulator environment.
 - crashes occasionally during playback
  - no video drawing
I'm going to see the problems tomorrow.
(Reporter)

Comment 27

5 years ago
Good idea to run this in the emulator. I didn't actually try that. The emulator uses software decoding which probably emits a different image format which we might not support (or not support correctly).

Comment 28

5 years ago
Created attachment 597288 [details] [diff] [review]
interdiff patch

interdiff patch to fix following problems on emulator
  - no video drawing
  - seek
597288 also fixes seeking issues on Akami as well.  Thanks!

I didn't pull in the OmxDecoder::ReadVideo() retry stuff into my build though (worked fine with out.)   What is this needed for BTW, and why 20?

Comment 30

5 years ago
> I didn't pull in the OmxDecoder::ReadVideo() retry stuff into my build
> though (worked fine with out.)   What is this needed for BTW, and why 20?

There are two reasons for "retry".
 - sw codec "i.e. AVCDecoder" frequently output consecutive zero sized Video frames.
 - handle format change (rarely happens)

On emulator, sw codecs are always used. In AVCDecoder's case, OmxDecoder often receives zero sized Video frames. But there are no big reason for "20". Yesterday, I watched about 10 consecutive zero sized Video frames from log output. Then I set tentatively maximum retry number to 20.

After ics, there is no AVCDecoder class. The class is replaced by SimpleSoftOMXComponent and SoftAVC. The situation might be changed. But I did not checked about it.

Comment 31

5 years ago
> On emulator, sw codecs are always used. In AVCDecoder's case, OmxDecoder
> often receives zero sized Video frames. But there are no big reason for
> "20". Yesterday, I watched about 10 consecutive zero sized Video frames from
> log output. Then I set tentatively maximum retry number to 20.

It seems that "AVCDecoder::read()" returns for each NAL. If a NAL does not include video frame(i.e. SPS, PPS, SEI, AUD, FILL, EOSEQ), AVCDecoder returns zero sized MediaBuffer.
Thanks for the info.  Do you know how Android normally deals with this?  Do they have a similar loop w/ "20" retry attempts?

Comment 33

5 years ago
> Thanks for the info.  Do you know how Android normally deals with this?  Do
> they have a similar loop w/ "20" retry attempts?

Android's implementation is use "for (;;)" loop.
It is implemented within "AwesomePlayer::onVideoEvent()"

code is like follwoing. This way is simpler than "loop w/MaxCount".

>         for (;;) {
>> //omit code
>            if (mVideoBuffer->range_length() == 0) {
>                // Some decoders, notably the PV AVC software decoder
>                // return spurious empty buffers that we just want to ignore.
>
>               mVideoBuffer->release();
>                mVideoBuffer = NULL;
>                continue;
>            }
>            break;
>        }
Cool, lets do that then.  The for(;;) is definitely better than a random number.
(Reporter)

Comment 35

5 years ago
Created attachment 598175 [details] [diff] [review]
patch

Latest and greatest patch. This is untested but fixes a couple conceptual issues. Amongst others we buffer VideoData before submitting it so we can calculate the proper end time, and we have to use end time when deciding whether we are within the threshold. We should fix httpd.js to send the right mime type instead of hacking away CORS and the content security checks (TODO tomorrow).
Attachment #596301 - Attachment is obsolete: true
Attachment #596304 - Attachment is obsolete: true
Attachment #596938 - Attachment is obsolete: true
Attachment #597288 - Attachment is obsolete: true
(Reporter)

Comment 36

5 years ago
I also removed Pin/Unpin for MPAPI. We can hide that in Gecko. Fewer things to get wrong in the plugin.
(Reporter)

Comment 37

5 years ago
Created attachment 598639 [details] [diff] [review]
Part 1: gonk patch

This patch fixes up a couple decoders in libstagefright that are broken in upstream Android. Commercial android builds probably already fix this.
Attachment #598175 - Attachment is obsolete: true
(Reporter)

Comment 38

5 years ago
Created attachment 598641 [details] [diff] [review]
Part 2: gecko patch

This patch fixes a bunch of stuff:
- fix mime type handling
- make sure httpd.js sends the right mime type
- fix a 14 year old bug in nsBinaryOutputStream (I am not even kidding) which killed httpd on short writes (pipe full)
- add a way to stop httpd.js from starting up, because it simply can't handle large files (it sucks large files into memory, twice)
- add support for octet files to omx-plugin when playing files from disk
- add proper slow path for all YUV video formats

With this patch video works great. There is no fast path for YUV format adjustment yet, so we suck 35% CPU time for a 320p H.264 video on akami, but even that is pretty respectable.
(Reporter)

Comment 39

5 years ago
Comment on attachment 598641 [details] [diff] [review]
Part 2: gecko patch

Bugzilla broken. I can't request review. Awesome. Will do via email.
(Reporter)

Comment 40

5 years ago
I landed part 1 on github. Only the gecko part remains now.
(Reporter)

Comment 41

5 years ago
Created attachment 599684 [details] [diff] [review]
Part 2: gecko patch
(Reporter)

Comment 42

5 years ago
Created attachment 599685 [details] [diff] [review]
Part 2: gecko patch
Attachment #599684 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #598641 - Attachment is obsolete: true
(Reporter)

Comment 43

5 years ago
This patch fixes some bad interaction between the media cache and the DataSource abstraction. This works now via http pretty well with a local web server. Haven't tested over the network with any significant latency.
(Reporter)

Comment 44

5 years ago
Created attachment 600724 [details] [diff] [review]
patch

Cleaned-up patch. Works great on akami/maguro. Samsung Galaxy S2 doesn't permit reading back frames. I will work on using the overlay hardware in a follow-up patch.
Attachment #598639 - Attachment is obsolete: true
Attachment #599685 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #600724 - Flags: review?(chris.double)
(Assignee)

Comment 45

5 years ago
Comment on attachment 600724 [details] [diff] [review]
patch

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

First look based on trying to build with patch applied, and running tests. More to come. A build with this patch applied, and plugins disabled, crashes when loading Ogg videos. It's missing the setting of mOffset and mSkip in the planar data. Something like:

diff --git a/content/media/ogg/nsOggReader.cpp b/content/media/ogg/nsOggReader.cpp
index 579c7a7..543da41 100644
--- a/content/media/ogg/nsOggReader.cpp
+++ b/content/media/ogg/nsOggReader.cpp
@@ -463,6 +463,7 @@ nsresult nsOggReader::DecodeTheora(ogg_packet* aPacket, PRInt64 aTimeThreshold)
       b.mPlanes[i].mHeight = buffer[i].height;
       b.mPlanes[i].mWidth = buffer[i].width;
       b.mPlanes[i].mStride = buffer[i].stride;
+      b.mPlanes[i].mOffset = b.mPlanes[i].mSkip = 0;
     }
 
     VideoData *v = VideoData::Create(mInfo,

::: configure.in
@@ +5028,5 @@
> +
> +if test -n "$MOZ_OMX_PLUGIN"; then
> +  AC_DEFINE(MOZ_OMX_PLUGIN)
> +fi
> +

This needs to be moved to after the MOZ_ARG_DISABLE_BOOL for 'media-plugins' and 'omx-plugin', otherwise compiling with "--disable-media-plugins" does not disable the media plugins and build fails. 

I think media plugins and omx-plugin should be disabled by default, and enabled explicitly. Or maybe disabled on all but Android/B2G?

::: content/media/plugins/nsMediaPluginHost.cpp
@@ +46,5 @@
> +#include "pratom.h"
> +#include "nsMediaPluginReader.h"
> +
> +#include "android/log.h"
> +#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "MediaPluginHost" , ## args)

Is nsMediaPluginHost and friends supposed to compile on non-android platforms? If so the above needs to be #ifdef'd out in some manner as "enable-media-plugins" fails to build on non-android platforms. Same comment applies to nsMediaPluginReader.cpp.
Attachment #600724 - Flags: review?(chris.double) → review-
(Reporter)

Comment 46

5 years ago
Created attachment 604305 [details] [diff] [review]
patch
Attachment #600724 - Attachment is obsolete: true
(Reporter)

Comment 47

5 years ago
Created attachment 604306 [details] [diff] [review]
patch

I would like to build MPAPI everywhere but we load the plugin on android only, and we disable MPAPI via pref by default.
Attachment #604305 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #604306 - Flags: review?(chris.double)
(Reporter)

Comment 48

5 years ago
Created attachment 604312 [details] [diff] [review]
patch

Use new CheckedInt64 return value.
Attachment #604306 - Attachment is obsolete: true
Attachment #604306 - Flags: review?(chris.double)
Attachment #604312 - Flags: review?
(Reporter)

Updated

5 years ago
Attachment #604312 - Attachment is patch: true
Attachment #604312 - Flags: review? → review?(chris.double)
(Reporter)

Comment 49

5 years ago
Created attachment 604317 [details] [diff] [review]
patch

don't configure video if we have audio only and vice versa
(Reporter)

Comment 50

5 years ago
Created attachment 604318 [details] [diff] [review]
patch

don't try to configure audio/video if there is no track for it
Created attachment 604319 [details] [diff] [review]
Decode mp3 audio

Fix for audio-only stream likely overlaps with just-posted patch.
(Reporter)

Updated

5 years ago
Attachment #604317 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #604318 - Attachment is obsolete: true
(Reporter)

Comment 52

5 years ago
Created attachment 604322 [details] [diff] [review]
only set audio/video format if we have corresponding tracks
Created attachment 604334 [details] [diff] [review]
Don't ReadVideo() if there's no video stream

This patch feels wrong but fixes another crasher.
(Reporter)

Comment 54

5 years ago
Ok thats wrong we shouldn't get there.
(Reporter)

Updated

5 years ago
Summary: support for libstagefright/OMX-based decoding → support for hardware-accelerated audio/video decoding (part 1: libstagefright/OMX support for Gonk)
Comment on attachment 604312 [details] [diff] [review]
patch

Clearly not in line with Mozilla policy.
Attachment #604312 - Flags: review?(chris.double) → review-
(Assignee)

Comment 56

5 years ago
Created attachment 605287 [details] [diff] [review]
Fix memory leak

There's a leak in the existing patch in attachment 604312 [details] [diff] [review]. The attached patch fixes it.

Comment 57

5 years ago
This is a DUP of bug 541286

Comment 58

5 years ago
If the reason is H.264, please enable only H.264 and MPEG2 (see bug 541286), but not all codecs that the platform might support. This and all the negative effects of enabling all codecs, esp. fragmentation and security problems, have been discussed extensively in bug 435339.
Blocks: 541286
(In reply to Ben Bucksch (:BenB) from comment #58)
> If the reason is H.264, please enable only H.264 and MPEG2 (see bug 541286),
> but not all codecs that the platform might support.

H.264 is the only encumbered video codec we need to support to be able to consume the same content Safari, IE9, Chrome and the Android stock browser consume. We should whitelist only H.264 of video codecs. Especially we shouldn't enabled the Web to become dependent on the MPEG-2 patent mess.

Comment 60

5 years ago
How about making this a pref? I.e. user can enable any format per pref, but only H.264 is enabled by default?

(MPEG2 is just the earlier version of the same ISO standard. There's tons of content in MPEG2 format out there - e.g. from TV / DVB.)
(In reply to Ben Bucksch (:BenB) from comment #60)
> How about making this a pref? I.e. user can enable any format per pref, but
> only H.264 is enabled by default?
> 
> (MPEG2 is just the earlier version of the same ISO standard. There's tons of
> content in MPEG2 format out there - e.g. from TV / DVB.)

There isn't a ton of MPEG-2 content out there *on the Web* *in HTML5 video*.

MPEG-2 isn't "just" an earlier version. It's a whole different patent situation. Different typical audio tracks. Different typical containers.

This bug is about Gonk and MPEG-2 TV/DVB stream over IP don't make sense at current mobile network speeds anyway. Let's continue this in dev-media if continuing this is necessary.

Comment 62

5 years ago
http://www.mythtv.org/wiki/MythWeb
And, yes, that makes a lot of sense on tablets at home.

My main point was: the end user can enable any format per pref, but only H.264 is enabled by default.
doublec, do you own this now?  I don't think Andreas is working on this anymore.
Depends on: 735769
doublec is on vacation, back in a couple of weeks.
Assignee: gal → chris.double

Comment 65

5 years ago
Created attachment 617798 [details] [diff] [review]
a patch adds hw video codec support on ics's b2g

while I tried to play H.264 video using hw codec on ics's B2G, I made a temporal patch.

the patch adds
 - support video size info(kKeyCropRect meta data) on ICS
 - support OMX_TI_COLOR_FormatYUV420PackedSemiPlanar color format

After ICS, actual video size info is provided by kKeyCropRect metadata.
Without the metadata, we can not get actual Video frame sizes in some cases.
when we did not add this, we faced segmentation fault under "videoImage->CopyData(data,..)" function.


I referenced following ICS's source code to add "OMX_TI_COLOR_FormatYUV420PackedSemiPlanar" support, 
 - \frameworks\base\media\libstagefright\colorconversion\ColorConverter.cpp
(Assignee)

Comment 66

5 years ago
Comment on attachment 604312 [details] [diff] [review]
patch

The newly added files need to have the boilerplate changed to use the new MPL 2 headers.

r+ with that and the change to fix the memory leak from attachment 605287 [details] [diff] [review].
Attachment #604312 - Flags: review- → review+
This can land.
I would prefer bug 735769 be investigated first, but we've been living with that regression for quite a while in b2g land so not a hard blocker.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #68)
> I would prefer bug 735769 be investigated first, but we've been living with
> that regression for quite a while in b2g land so not a hard blocker.

The work in this bug is really important;  it's one of our Q2 goals (delivering Platform Decoder/MPAPI support) and will be strongly desired for K9o.  Do we need to resolve Bug 735769 before we ship Platform Decoders (MPAPI support) in B2G and Android?  

The answer may well be yes, but I looked at the comments in 735769, and they were pretty sparse with not much context.  Can you expand on what problems Bug 735769 causes?  (Either here or in Bug 735769.) Thanks.
(Reporter)

Comment 70

5 years ago
#0  0x40950e14 in nsMediaPluginReader::GetBuffered (this=0x46ec1200, 
    aBuffered=<value optimized out>, aStartTime=<value optimized out>)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/content/media/plugins/nsMediaPluginReader.cpp:327
#1  0x409434fa in nsBuiltinDecoderStateMachine::GetBuffered (
    this=<value optimized out>, aBuffered=0xbedc24c8)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/content/media/nsBuiltinDecoderStateMachine.cpp:2165
#2  0x40941f0c in nsBuiltinDecoder::GetBuffered (this=<value optimized out>, 
    aBuffered=0xbedc2450)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/content/media/nsBuiltinDecoder.h:485
#3  0x40944214 in nsBuiltinDecoderStateMachine::NotifyDataArrived (
    this=0x45eb5000, aBuffer=<value optimized out>, 
    aLength=<value optimized out>, aOffset=0)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/content/media/nsBuiltinDecoderStateMachine.cpp:1202
#4  0x40941f76 in nsBuiltinDecoder::NotifyDataArrived (
    this=<value optimized out>, aBuffer=0xbedc2450 "", aLength=0, aOffset=0)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/content/media/nsBuiltinDecoder.h:506
#5  0x40947cce in mozilla::ChannelMediaResource::CopySegmentToCache (
    aInStream=<value optimized out>, aClosure=0xbedc25e4,
   aFromSegment=0x46fc9000 "", aToOffset=<value optimized out>, aCount=512, 
    aWriteCount=0xbedc25a4)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/content/media/MediaResource.cpp:375
#6  0x40c6baae in nsPipeInputStream::ReadSegments (this=0x476e8708, 
    writer=0x40947ca5 <mozilla::ChannelMediaResource::CopySegmentToCache(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*)>, 
    closure=0xbedc25e4, count=512, readCount=0xbedc25ec)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/xpcom/io/nsPipe3.cpp:799
#7  0x40947312 in mozilla::ChannelMediaResource::OnDataAvailable (
    this=<value optimized out>, aRequest=<value optimized out>, 
    aStream=0x476e8708, aCount=512)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/content/media/MediaResource.cpp:408
#8  0x4094734c in mozilla::ChannelMediaResource::Listener::OnDataAvailable (
    this=<value optimized out>, aRequest=0xbedc2450, 
    aContext=<value optimized out>, aStream=<value optimized out>, aOffset=0, 
    aCount=512)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/content/media/MediaResource.cpp:127
#9  0x407e1020 in nsHTMLMediaElement::MediaLoadListener::OnDataAvailable (
    this=<value optimized out>, aRequest=0xbedc2450, aContext=0x0, 
    aStream=0x0, aOffset=0, aCount=512)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/content/html/content/src/nsHTMLMediaElement.cpp:395
#10 0x4051cffa in nsUnknownDecoder::FireListenerNotifications (
    this=0x432f0ca0, request=0x47b8fc30, aCtxt=0x0)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/netwerk/streamconv/converters/nsUnknownDecoder.cpp:653
#11 0x4051d0a8 in nsUnknownDecoder::OnDataAvailable (this=0x432f0ca0, 
    request=0x47b8fc30, aCtxt=0x0, aStream=0x476e99c8, aSourceOffset=512, 
    aCount=392704)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/netwerk/streamconv/converters/nsUnknownDecoder.cpp:185
#12 0x40554d8e in nsHttpChannel::OnDataAvailable (this=0x47b8fc00, 
    request=<value optimized out>, ctxt=<value optimized out>, 
    input=0x476e99c8, offset=0, count=393216)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/netwerk/protocol/http/nsHttpChannel.cpp:4604
#13 0x404feb36 in nsInputStreamPump::OnStateTransfer (this=0x47af6f60)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/netwerk/base/src/nsInputStreamPump.cpp:518
#14 0x404fec46 in nsInputStreamPump::OnInputStreamReady (this=0x47af6f60, 
    stream=<value optimized out>)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/netwerk/base/src/nsInputStreamPump.cpp:402
#15 0x40c6c064 in nsInputStreamReadyEvent::Run (this=0x47a24260)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/xpcom/io/nsStreamUtils.cpp:193
#16 0x40c76ae4 in nsThread::ProcessNextEvent (this=0x41b12460, 
    mayWait=<value optimized out>, result=0xbedc27a7)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/xpcom/threads/nsThread.cpp:656
#17 0x40c550d8 in NS_ProcessNextEvent_P (thread=0x0, mayWait=true)
    at /local/mnt/workspace/dwilson/ics.b2g/out/target/product/msm7627a/obj/objdir-gecko/xpcom/build/nsThreadUtils.cpp:245
#18 0x40c76c72 in nsThread::Shutdown (this=0x47ddf700)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/xpcom/threads/nsThread.cpp:498
#19 0x405053be in nsRunnableMethodImpl<void (nsPACMan::*)(), true>::Run (
    this=<value optimized out>) at ../../../dist/include/nsThreadUtils.h:345
#20 0x40c76ae4 in nsThread::ProcessNextEvent (this=0x41b12460, 
    mayWait=<value optimized out>, result=0xbedc282f)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/xpcom/threads/nsThread.cpp:656
#21 0x40c550d8 in NS_ProcessNextEvent_P (thread=0x0, mayWait=false)
    at /local/mnt/workspace/dwilson/ics.b2g/out/target/product/msm7627a/obj/objdir-gecko/xpcom/build/nsThreadUtils.cpp:245
#22 0x40be8828 in mozilla::ipc::MessagePump::Run (this=0x41b0f1f0,
   aDelegate=0x41b1a0e0)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/ipc/glue/MessagePump.cpp:110
#23 0x40c9a36a in MessageLoop::RunInternal (this=0xbedc2450)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/ipc/chromium/src/base/message_loop.cc:208
#24 0x40c9a44a in MessageLoop::RunHandler (this=0x41b1a0e0)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/ipc/chromium/src/base/message_loop.cc:201
#25 MessageLoop::Run (this=0x41b1a0e0)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/ipc/chromium/src/base/message_loop.cc:175
#26 0x40b740d4 in nsBaseAppShell::Run (this=0x42392340)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/widget/xpwidgets/nsBaseAppShell.cpp:189
#27 0x40a9fc0e in nsAppStartup::Run (this=0x4235efd0)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/toolkit/components/startup/nsAppStartup.cpp:295
#28 0x404e62da in XREMain::XRE_mainRun (this=0xbedc2a74)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/toolkit/xre/nsAppRunner.cpp:3768
#29 0x404e8d84 in XREMain::XRE_main (this=0xbedc2a74, 
    argc=<value optimized out>, argv=0xbedc4c64, 
    aAppData=<value optimized out>)
at /local/mnt/workspace/dwilson/ics.b2g/gecko/toolkit/xre/nsAppRunner.cpp:3845
#30 0x404e8efe in XRE_main (argc=1, argv=0xbedc4c64, aAppData=0xab80)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/toolkit/xre/nsAppRunner.cpp:3921
#31 0x0000897e in do_main (argc=1, argv=0xbedc4c64)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/b2g/app/nsBrowserApp.cpp:186
#32 main (argc=1, argv=0xbedc4c64)
    at /local/mnt/workspace/dwilson/ics.b2g/gecko/b2g/app/nsBrowserApp.cpp:269

Comment 71

5 years ago
Created attachment 623069 [details] [diff] [review]
temporary patch: no video frame copy rendering

Current "OmxPlugin.cpp" implementaion is not good enough from performance and memory usage.
Even when using hw decoder, cpu reads video frames from video buffers and copy the buffers to memories,
then the video data are uploaded to OpenGL texture when rendering.
This spend cpu time and large amount of memories.
and need to care about platform dependent proprietary color formats!

I played some H264 videos on my phone (omap dualcore, ics).
  - H.264 600*480 30fps: spent 50% cpu time.
  - H.264 720P 24fps: soon after start playback, video rendering stopped. 


IMHO, b2g need to solve this problem.

Then I wrote a tentative patch. the patch has a lot of ploblems, currently.
But It could avoud video buffer copy, and save memories.
The patch is so clumsy. But this way of video rendering is necessay.

Thought about this?


* characteristics of the patch

- use ANativeWindow for GraphicBuffer allocation
     the video output is written to GraphicBuffer.
- do not copy video frames by using cpu, but deliver the buffers to ImageLayerOGL.
- the buffer(GraphicBuffer) are directly bound to OpenGL texture() and rendered
- no need to care about proprietary color formats


*limitations/problems (write just some...)

 + [1] disable jemalloc tentatively
     GraphicBuffer make allocator/deallocator mismatch. refer bug 740997
 + [2] implementation is so temporal.
 + [3] do not destruct objects correctly.
 + [4] build only on ics platform(could not build on gingerbread)
 + [5] do not support to overlay 
     need to add support of HWComposer to LayerManagerOGL and ImageLayerOGL.
 + [6] could play only a few sizes of videos(i.e. 720p on my ics phone)

do not handle that omap hw codec output do not map all decoded video output buffer areas.
the codec memory map all actual video area (crop region).
but outside of the crop region is not always memory mapped.
attachment 617798 [details] [diff] [review] avoid the problem by only read actual video area (crop region).
 but in this patch , I could not find a way to solve this.

     on my phone, I could played 720p video. so, I checked the video playback by using 720p video.

Comment 72

5 years ago
by using attachment 623069 [details] [diff] [review] on my phone (omap dualcore, ics).

H.264 720P 24fps : spent about 20% cpu time during playback.
There may be lots of issues with your patch, but the performance improvement looks to be significant - copying video buffers is always to be avoided when possible.  What's the delta in performance at 640x480 30fps?

These chips (especially DSP-based ones like OMAP) often have all sorts of handy hardware engines for memory copies, etc; doing them in the CPU simply sucks.  (Hell, copying video data in Desktop sucks too if can possibly be avoided.)

It sounds like (for B2G) the jemalloc issue can be resolved per the bug you reference.

Updated

5 years ago
Attachment #623069 - Attachment description: temporal patch: no video frame copy rendering → temporary patch: no video frame copy rendering

Comment 74

5 years ago
>What's the delta in performance at 640x480 30fps?

because of the patch's problem, 640x480 30fps could not be played...Video out buffers starved soon after start playback for some reason. then OmxCodec::read() failed because of timeout. I could check the delta on smaller video size.

video size:with copy->no copy
-320*240   :cpu 29%  -> cpu 17%
-720p      :video update stopped -> cpu 20%

Comment 75

5 years ago
Created attachment 623556 [details] [diff] [review]
temporary patch: no video frame copy rendering


just fix a small problem of the attachment 623069 [details] [diff] [review].
I had selected a wrong "gecko/toolkit/library/Makefile.in" file.
Attachment #623069 - Attachment is obsolete: true

Comment 76

5 years ago
by modifying the code, 600*480 could be played with the patch.

video size           :with copy ->no copy
-H.264 320*240 25fps :cpu 29%   -> cpu 17%
-H.264 600*480 30fps :cpu 50%   -> cpu 18% 
-H.264 720P 24fps    :video update stopped -> cpu 20%

Comment 77

5 years ago
Created attachment 623590 [details] [diff] [review]
temporary patch: no video frame copy rendering

temporary fix for 600*480 video play problem.
comment out following code. the change is also tentative.

nsMediaPluginReader::DecodeVideoFrame()

//    if (mLastVideoFrame->mEndTime < aTimeThreshold) {
//      delete mLastVideoFrame;
//      mLastVideoFrame = NULL;
//      continue;
//    }
Attachment #617798 - Attachment is obsolete: true
Attachment #623556 - Attachment is obsolete: true

Comment 78

5 years ago
Created attachment 623971 [details] [diff] [review]
temporary patch: no video frame copy rendering

update attachment 623590 [details] [diff] [review].

fix buffer starvation during playing 600*480 video.

suppress actual number of video frames owned by the OmxCodec' client-side at most to two, otherwise OmxCodec starves video buffer during playing 600*480 video on my phone. 
At first I tried to increase number of video output buffer, but it's request is rejected by TI OMX component.

i.e: stagefright's AwesomePlayer concurrently own at most only one video frame.
Attachment #623590 - Attachment is obsolete: true
blocking-kilimanjaro: --- → ?
(Assignee)

Comment 79

5 years ago
Are we going to land the existing patch for libstagefright support and look at the no-copy work in a separate bug, or should we wait for the no-copy work?
(Reporter)

Comment 80

5 years ago
I would definitely do this incrementally. Land what we have, continue this awesome zero-copy work in a new bug. Also, the current patch needs an additional fix in GetBuffered() to return 0 if !mPlugin otherwise we crash on ICS. We seem to call GetBuffered from two threads. Trying to instantiate the plugin in GetBuffered() deadlocks. Maybe doublec can help figuring out why. We couldn't explain it at the work week.
We call GetBuffered() from inside the state machine thread and decode threads in order to figure out whether we have enough undecoded data remaining to play without stopping. It can also be called on the main thread by JS.
(Reporter)

Comment 82

5 years ago
Can you audit the code to make sure that synchronization is sane? At the very least we access mPlugin from 2 threads without synchronizing properly. Hellgrind would probably barf at this.

Comment 83

5 years ago
Correction about following.

>  + [6] could play only a few sizes of videos(i.e. 720p on my ics phone)
> omap hw codec output do not map all decoded video output buffer areas.
> the codec memory map all actual video area (crop region).
> but outside of the crop region is not always memory mapped.
> attachment 617798 [details] [diff] [review] avoid the problem by only read
> actual video area (crop region).
>  but in this patch , I could not find a way to solve this.

I have to say it was my misunderstanding. There is no problem like it when do rendering by directly bound GraphicBuffer to texture.

By using attachment 623069 [details] [diff] [review], segmentation faults happened depends on the video sizes, then I made wrong assumption...
Attachment 623971 [details] [diff] do not have such restriction. There is currently no restriction like [6]. It seems that the faults happend because of wrong MediaBuffer handling.

i.e: within nsMediaPluginReader::DecodeVideoFrame()

>    if (mLastVideoFrame->mEndTime < aTimeThreshold) {
>      delete mLastVideoFrame;
>      mLastVideoFrame = NULL;
>      continue;
>    }

the above code is not a problem in "with video copy" case.
but a problem in "no video copy" case, then changed the code like following.

>    if (mLastVideoFrame->mEndTime < aTimeThreshold) {
>      delete mLastVideoFrame;
>      mLastVideoFrame = NULL;
>      mLastVideoFrame = v; //update mLastVideoFrame
>      continue;
>    }

Comment 84

5 years ago
attachment 623971 [details] [diff] [review] could not build on b2g.

My local ics-b2g code enviroment modified differently from normal b2g.
That cause to making wrong patch.
I am going to check the patch and update, soon.

Comment 85

5 years ago
Created attachment 624277 [details] [diff] [review]
temporary patch: no video frame copy rendering

update attachment 623971 [details] [diff] [review]

add following to the patch
 - /glue/gonk-ics/frameworks/base/include/gui/ISurfaceTexture.h
 - /glue/gonk-ics/frameworks/base/include/ui/GraphicBuffer.h
Attachment #623971 - Attachment is obsolete: true
Created attachment 624281 [details] [diff] [review]
Patch

Took out hunk which reduces media cache size due to bug 755533.

Reducing media cache size split out into bug 755609.
Attachment #604312 - Attachment is obsolete: true
Attachment #624281 - Flags: review?(roc)

Comment 87

5 years ago
Created attachment 624282 [details] [diff] [review]
temporary patch: no video frame copy rendering

update again...

I moved following call to wrong position and move back.

android::sp<android::GraphicBuffer> activeBuffer = surfaceTexture->getCurrentBuffer();
Attachment #624277 - Attachment is obsolete: true
Comment on attachment 624281 [details] [diff] [review]
Patch

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

Let's land this. I'm rubber-stamping since doublec already reviewed.
Attachment #624281 - Flags: review?(roc) → review+
(Assignee)

Comment 89

5 years ago
Comment on attachment 605287 [details] [diff] [review]
Fix memory leak

Before landing needs attachment 605287 [details] [diff] [review] as well to fix memory leak in original patch.
Attachment #605287 - Flags: review?(cpearce)
Comment on attachment 605287 [details] [diff] [review]
Fix memory leak

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

Maybe we should just fold this patch into the former patch?

::: content/media/plugins/nsMediaPluginHost.cpp
@@ +166,5 @@
>    aDecoder->DestroyDecoder(aDecoder);
>    delete aDecoder;
>  }
>  
> +nsMediaPluginHost *sMediaPluginHost = nsnull;

I'd add a blank line here between these two lines for readability.
Attachment #605287 - Flags: review?(cpearce) → review+
(Assignee)

Comment 91

5 years ago
Created attachment 625546 [details] [diff] [review]
Part 1: Configure/Toolkit changes

Splits configure/build changes from main patch. Changes 'disable-media-plugins' to 'enable-media-plugins' so they're disabled by default.

This patch adds:

* --enable-media-plugins to media the content/media/plugin code
* --enable-omx-plugin to build the omx-plugin.so shared library
* --with-android-source for setting the director for the android source code (used for non-b2g android builds)
Attachment #625546 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 92

5 years ago
Created attachment 625547 [details] [diff] [review]
Part 2: Media plugin/Omx Plugin support for mp4/mp3 playback via libstagefright

Splits out the non-configure build changes from the original patch for separate review.

* Rolls up previously individual patches.
* Moves omx-plugin from toolkit/mozapps/omx-plugin to media/omx-plugin
* Changes MPL license in new files to MPL2

Review carried forward.
Attachment #604319 - Attachment is obsolete: true
Attachment #604322 - Attachment is obsolete: true
Attachment #604334 - Attachment is obsolete: true
Attachment #605287 - Attachment is obsolete: true
Attachment #624281 - Attachment is obsolete: true
Attachment #625547 - Flags: review+
(Assignee)

Comment 93

5 years ago
Comment on attachment 625546 [details] [diff] [review]
Part 1: Configure/Toolkit changes

A few changes to this are needed - a new patch will be up shortly.
Attachment #625546 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 94

5 years ago
Created attachment 625897 [details] [diff] [review]
Part 1: Configure/Toolkit changes

Fixes issues with previous patch. Previous comment duplicated below.

Splits configure/build changes from main patch. Changes 'disable-media-plugins' to 'enable-media-plugins' so they're disabled by default.

This patch adds:

* --enable-media-plugins to media the content/media/plugin code
* --enable-omx-plugin to build the omx-plugin.so shared library
* --with-android-source for setting the director for the android source code (used for non-b2g android builds)
Attachment #625546 - Attachment is obsolete: true
Attachment #625897 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 95

5 years ago
Created attachment 625898 [details] [diff] [review]
Part 2: Media plugin/Omx Plugin support for mp4/mp3 playback via libstagefright

Tweaks OmxPlugin to build on andreasgal/B2G gingerbread builds (ie. akami) as well as mozilla-b2g/B2G ICS builds (eg. Nexus S).
Attachment #625547 - Attachment is obsolete: true
Attachment #625898 - Flags: review+
Blocks: 757341
(Assignee)

Comment 96

5 years ago
Created attachment 626297 [details] [diff] [review]
Part 2: Media plugin/Omx Plugin support for mp4/mp3 playback via libstagefright

Fixes bitrot from the 'seeking in buffered ranges' changes that landed recently.
Attachment #625898 - Attachment is obsolete: true
Attachment #626297 - Flags: review+

Comment 97

5 years ago
Created attachment 626354 [details] [diff] [review]
temporary patch: no video frame copy rendering

update attachment 624282 [details] [diff] [review]

update following two.

- comment out the following within SurfaceTextureGonk::updateTexImage()
  gl->fTexParameteriv(LOCAL_GL_TEXTURE_EXTERNAL_OES, LOCAL_GL_TEXTURE_CROP_RECT_OES, crop);

GL_TEXTURE_CROP_RECT_OES is not necessary.
And it seems that GL_TEXTURE_CROP_RECT_OES do not work for GL_TEXTURE_EXTERNAL_OES.

- add some code within ImageLayerOGL::RenderLayer() to care the case that a texture crop of SurfaceTextureGonk is [0,0,0,0] .

when a texture crop of SurfaceTextureGonk is [0,0,0,0], all region of GraphicBuffer/texture is valid. I recognized it from bug 757341
You could understand this from SurfaceTexture::computeCurrentTransformMatrix().

http://androidxref.com/source/xref/frameworks/base/libs/gui/SurfaceTexture.cpp#804
Attachment #624282 - Attachment is obsolete: true

Updated

5 years ago
blocking-kilimanjaro: ? → +

Updated

5 years ago
Blocks: 746388
Chris, 
   I think I am almost ready to begin testing this patch. 

If you could highlight some areas where you feel we need coverage, and across which operating systems. That would be great. 

https://developer.mozilla.org/en/DOM/HTMLMediaElement

Assuming that we'd want to test against the Spec, and make sure all of the properties work as expected. 

Any clues as to how to pick up on any memory leaks would be much appreciated. 

I am starting to put together a test page / content which will allow me to exercise different features of the api.

Assuming we care about mobile / b2g at the higher levels of priority.

Updated

5 years ago
Whiteboard: [qa+]
(Assignee)

Comment 99

5 years ago
We have a bunch of mochitests for video in content/media/tests. If  they could be run on mobile, or some way of running these to test the stagefright backend, it'd do wonders for making sure the code is correct.
(Assignee)

Comment 100

5 years ago
Comment on attachment 625897 [details] [diff] [review]
Part 1: Configure/Toolkit changes

Requesting review from khuey in case ted is swamped.
Attachment #625897 - Flags: review?(khuey)
Whiteboard: [qa+] → [qa+][b2g:blocking+]
Blocks: 759506
Testing will move down two paths.  One manual, one automated. 

There are currently a large repository of media tests insited content/media/test. 
The goal will be to add tests in there and run the tests across desktop and b2g. 
Testing across the more popular chipsets. 

Manual Testing.  I have put together a dummy test side with audio / video clips of different sizes / encoding rates.  http://people.mozilla.com/~dclarke/
Generally there will be some set of manual tests that will be iterated through. 

My next step is to build a version of B2G with GB as the kernel + above patches, and get to the state where we can begin manual tests.
(Assignee)

Comment 102

5 years ago
You'll probably need to remove the use of <select> in your test as it appears to not work in the b2g browser. Possibly bug 759511.

Updated

5 years ago
Flags: in-testsuite?
Flags: in-moztrap?
(Assignee)

Comment 103

5 years ago
Created attachment 628510 [details] [diff] [review]
Part 3: audio channel count initialized with wrong value - r=eflores

OmxPlugin is picking up an incorrect channel count when initializing the audio.
Attachment #628510 - Flags: review?(eflores)
Attachment #628510 - Flags: review?(eflores) → review+
(Assignee)

Updated

5 years ago
Blocks: 759945
(Assignee)

Updated

5 years ago
Blocks: 759948
(Assignee)

Comment 104

5 years ago
Comment on attachment 625897 [details] [diff] [review]
Part 1: Configure/Toolkit changes

Removed :khuey since he's on vacation.
Attachment #625897 - Flags: review?(khuey)
Comment on attachment 625897 [details] [diff] [review]
Part 1: Configure/Toolkit changes

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

Mostly nits.

::: configure.in
@@ +221,5 @@
>  
> +MOZ_ARG_WITH_STRING(android-source,
> +[  --with-android-source=DIR
> +                          location where the Android source can be found],
> +    android_source=$withval)

Since this is really for gonk, and since I don't think we should encourage that being used for android, I'd say please rename to --with-gonk-source (and move it with other gonk stuff)

@@ +480,5 @@
>  fi
>  
>  AC_SUBST(ANDROID_NDK)
>  AC_SUBST(ANDROID_TOOLCHAIN)
> +AC_SUBST(ANDROID_SOURCE)

This variable is not added to autoconf.mk.in.

@@ +5661,5 @@
> +[  --enable-omx-plugin      Enable building OMX plugin (android)],
> +    MOZ_OMX_PLUGIN=1,
> +    MOZ_OMX_PLUGIN=)
> +
> +if test -n "$MOZ_OMX_PLUGIN"; then

Please add a check that OS_TARGET is Android (or whatever it is for B2G).
Attachment #625897 - Flags: review?(ted.mielczarek) → review+
(In reply to Mike Hommey [:glandium] from comment #105)
> Comment on attachment 625897 [details] [diff] [review]
> Part 1: Configure/Toolkit changes
> 
> Review of attachment 625897 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly nits.
> 
> ::: configure.in
> @@ +221,5 @@
> >  
> > +MOZ_ARG_WITH_STRING(android-source,
> > +[  --with-android-source=DIR
> > +                          location where the Android source can be found],
> > +    android_source=$withval)
> 
> Since this is really for gonk, and since I don't think we should encourage
> that being used for android, I'd say please rename to --with-gonk-source
> (and move it with other gonk stuff)

Oh wait, I didn't see that:
> * --with-android-source for setting the director for the android source
> code (used for non-b2g android builds)

So even better, just don't put the configure flag at all, but still AC_SUBST(ANDROID_SOURCE). People who want to test it can still use export ANDROID_SOURCE=/path/ in their mozconfig.
blocking-basecamp: --- → +
Whiteboard: [qa+][b2g:blocking+] → [qa+]
(Assignee)

Comment 107

5 years ago
Thanks for the review! I'll address your comments in the next patch.

(In reply to Mike Hommey [:glandium] from comment #105)
> that being used for android, I'd say please rename to --with-gonk-source
> (and move it with other gonk stuff)

As per your comment 106 I'll remove this entirely and let people export ANDROID_SOURCE if they need it.

> >  AC_SUBST(ANDROID_NDK)
> >  AC_SUBST(ANDROID_TOOLCHAIN)
> > +AC_SUBST(ANDROID_SOURCE)
> 
> This variable is not added to autoconf.mk.in.

It's already in autoconf.mk.in as it's set and used in gonk builds.
(Assignee)

Comment 108

5 years ago
Created attachment 629012 [details] [diff] [review]
Part 1: Configure/Toolkit changes

Address review comments, carrying forward r+
Attachment #625897 - Attachment is obsolete: true
Attachment #629012 - Flags: review+
(Assignee)

Comment 109

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ac6a5b9aea
https://hg.mozilla.org/integration/mozilla-inbound/rev/85a3f4b90adc
https://hg.mozilla.org/integration/mozilla-inbound/rev/09da0033b3a0
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/d9ac6a5b9aea
https://hg.mozilla.org/mozilla-central/rev/85a3f4b90adc
https://hg.mozilla.org/mozilla-central/rev/09da0033b3a0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Have been trying to get mp4 content to play in browser on B2G, and have not had much success. 
I have verified that these patches have been applied. 

http://www.808.dk/pics/video/gizmo.mp4

Can someone verify that with these patches you can play the h264 encoded content above ?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 112

5 years ago
Chris whats up here? Our silicon vendor friends were complaining about horribly low frame rates too (5FPS etc).
(Assignee)

Comment 113

5 years ago
(In reply to Andreas Gal :gal from comment #112)
> Chris whats up here? Our silicon vendor friends were complaining about
> horribly low frame rates too (5FPS etc).

No idea. What device? Do they have an example video? Are they accessing over the network or from the offline cache? Buffering in the patch is pretty bad so may be related to that.
(Assignee)

Comment 114

5 years ago
(In reply to dclarke@mozilla.com from comment #111)
> 
> Can someone verify that with these patches you can play the h264 encoded
> content above ?

Video plays for me but sound is at the wrong rate. Note that currently you need to have the media in a <video> or <audio> element, you can't use a direct link. Try:

http://cd.pn/t2
Status: REOPENED → ASSIGNED
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Created attachment 630008 [details]
screenshot of no video

Hi Chris, I'm unable to view your test video from the last comment.   Running on a Mac nightly, built from changeset: http://hg.mozilla.org/mozilla-central/rev/dd6ec482a85d.

screnshot of video status spinning.

Is there dependent fixes that Firefox Nightly needs to add before this streams correctly?
(In reply to Tony Chung [:tchung] from comment #115)
> Created attachment 630008 [details]
> screenshot of no video
> 
> Hi Chris, I'm unable to view your test video from the last comment.  
> Running on a Mac nightly, built from changeset:
> http://hg.mozilla.org/mozilla-central/rev/dd6ec482a85d.
> 
> screnshot of video status spinning.
> 
> Is there dependent fixes that Firefox Nightly needs to add before this
> streams correctly?

Nevermind, its on b2g phone only.   confirmed, playback is really slow there.
(Assignee)

Comment 117

5 years ago
(In reply to Tony Chung [:tchung] from comment #116)
> 
> Nevermind, its on b2g phone only.   confirmed, playback is really slow there.

Audio playback is slow, right? Video playback is fine?
Confirmed audio playback is slow, also the controls for volume / rewind didn't seem to respond. Is this expected ?
(Assignee)

Comment 119

5 years ago
Volume only mutes/unmutes as far as I know - and works for me. I don't see a rewind, only a seek bar, which works for me.

Can you raise a bug for the slow audio playback issue?
I see that bug 761762 has been filed for the slow audio playback issue.  Thanks, David!  

Are there any additional bugs here that need to be filed (like unable to control the volume on the phone or unable to rewind the video)?  I don't have a B2G phone to look myself -- so if there there are additional bugs here, can we file them?   Thanks!
(Assignee)

Comment 121

5 years ago
Yes please raise bugs for issues identified in comment 112 and comment 118 (the rewind issue, the slow audio has a patch in bug 761762). I don't see these and would like more detail. Other than those reports I don't know of any other user facing issue with B2G MP4 playback so if you know any, please file.
Maire, ok , but i noticed was that there is volume control on the samsung sII on the left hand side of the device, so having one on the content might not be necessary. 

Rewind issue was more related to me getting used to B2G device interface. It works for me now.
(In reply to dclarke@mozilla.com from comment #122)
> Maire, ok , but i noticed was that there is volume control on the samsung
> sII on the left hand side of the device, so having one on the content might
> not be necessary. 
> 
> Rewind issue was more related to me getting used to B2G device interface. It
> works for me now.

Great, David!  Really I meant to ask if there were any bugs not filed that should be.  It sounds like you believe all the open issues have been filed as bugs (which is excellent -- thank you!)  

And I agree that as long as there is at least one good way to easily and intuitively adjust the volume on the phone we are shipping (like volume control buttons on the side that are known to work), then I agree we're good.  

To Chris D's (doublec) question: Have you seen the problems that Andreas mentioned in comment 112 ("horribly low frame rates.. (5fps)") at any time in your testing?  It sounds like you haven't?  

Have you been doing all of your testing on the Samsung SII or have you been testing on other phones as well? 

Thanks!
Yes I have, but I assume we are fixing that in the follow up bug, where the hardware accelerated version is being tracked. 

people.mozilla.com/~dclarke/b.html <--- I have a 720p sample. 

I think 720p on mobile is fairly standard nowadays. 

I am filing bugs as i find / think of them.
(In reply to dclarke@mozilla.com from comment #124)
> Yes I have, but I assume we are fixing that in the follow up bug, where the
> hardware accelerated version is being tracked.
 
Is that Bug 759506?  If so, could you update that bug with your comments on what you see?  We don't have many details in the bug on exactly what the problem is from the user's point of view, and I believe the developers who are working the problem (in NZ) aren't seeing the problem themselves yet.

And is all your testing on the Samsung SII so far?

Thanks so much, David!
(Assignee)

Comment 126

5 years ago
I'm not aware of a "hardware accelerated version" followup bug. Do you have a bug number? Bug 759506 optimizes code to reduce copying afaik. Maire, I have not seen horribly low frame rates but I haven't been testing large videos yet.

David, when you see things like that can you raise a bug specifically for it? We can always dupe it if it's covered by something else. 

Maire, all my testing is on a Galaxy Nexus S.

Updated

5 years ago
Flags: in-moztrap? → in-moztrap?(dclarke)
Flags: in-moztrap?(dclarke) → in-moztrap?
Flags: in-testsuite?
Flags: in-moztrap?
David please do not remove the MozTrap flag in this case - we need someone to add MozTrap test cases for this feature, especially since manual test cases will need to exist that are cross-functional across B2G and Android.

We should get automation in place as well, so the in-testsuite? flag makes sense.
Flags: in-testsuite?
Flags: in-moztrap?
Well I actually do mind it being added because this is not really a feature that is testable. Maybe if you had worked in this area, or understood the bug better that would help.
Flags: in-testsuite?
Flags: in-moztrap?
Blocks: 774552
No longer blocks: 774552

Updated

5 years ago
Depends on: 789075
Whiteboard: [qa+] → [qa+][caf:helping]
You need to log in before you can comment on or make changes to this bug.