Last Comment Bug 714408 - support for hardware-accelerated audio/video decoding (part 1: libstagefright/OMX support for Gonk)
: support for hardware-accelerated audio/video decoding (part 1: libstagefright...
Status: RESOLVED FIXED
[qa+][caf:helping]
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All Android
: -- normal with 5 votes (vote)
: mozilla15
Assigned To: cajbir (:cajbir)
:
Mentors:
http://download.blender.org/peach/big...
Depends on: 735769 789075
Blocks: k9o-web-platform 541286 b2g-product-phone 757341 759506 759945 759948
  Show dependency treegraph
 
Reported: 2011-12-30 19:28 PST by Andreas Gal :gal
Modified: 2015-11-20 13:42 PST (History)
56 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
patch (50.79 KB, patch)
2011-12-30 19:28 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (94.05 KB, patch)
2012-01-04 06:31 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (94.05 KB, patch)
2012-01-06 02:11 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (94.07 KB, patch)
2012-01-12 22:13 PST, Andreas Gal :gal
no flags Details | Diff | Review
make patch to b2g's source code (93.59 KB, patch)
2012-01-29 18:20 PST, Sotaro Ikeda
no flags Details | Diff | Review
patch to b2g (MediaPlayerService disabled) (93.96 KB, patch)
2012-01-30 21:58 PST, Sotaro Ikeda
no flags Details | Diff | Review
Part 1: gonk patch to fix some quirks and disable old drivers for akami (3.01 KB, patch)
2012-02-11 04:40 PST, Andreas Gal :gal
no flags Details | Diff | Review
Part 2: MPAPI and omx-plugin for B2G (19.12 KB, patch)
2012-02-11 04:41 PST, Andreas Gal :gal
no flags Details | Diff | Review
Part 3: enable omx-plugin in build config (371 bytes, patch)
2012-02-11 04:41 PST, Andreas Gal :gal
no flags Details | Diff | Review
test case for B2G, install as /data/local/homescreen.html along with sample.mp4 (275 bytes, text/html)
2012-02-11 04:42 PST, Andreas Gal :gal
no flags Details
informational patch (3.00 KB, patch)
2012-02-12 22:14 PST, Sotaro Ikeda
no flags Details | Diff | Review
part 2: gecko patch (43.55 KB, patch)
2012-02-13 23:09 PST, Andreas Gal :gal
no flags Details | Diff | Review
part 2: gecko patch (78.80 KB, patch)
2012-02-14 00:01 PST, Andreas Gal :gal
no flags Details | Diff | Review
interdiff patch (4.56 KB, patch)
2012-02-14 20:32 PST, Sotaro Ikeda
no flags Details | Diff | Review
patch (77.19 KB, patch)
2012-02-17 01:54 PST, Andreas Gal :gal
no flags Details | Diff | Review
Part 1: gonk patch (3.81 KB, patch)
2012-02-19 01:57 PST, Andreas Gal :gal
no flags Details | Diff | Review
Part 2: gecko patch (81.96 KB, patch)
2012-02-19 02:05 PST, Andreas Gal :gal
no flags Details | Diff | Review
Part 2: gecko patch (2.30 KB, patch)
2012-02-22 10:26 PST, Andreas Gal :gal
no flags Details | Diff | Review
Part 2: gecko patch (80.05 KB, patch)
2012-02-22 10:27 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (78.38 KB, patch)
2012-02-25 15:31 PST, Andreas Gal :gal
cajbir.bugzilla: review-
Details | Diff | Review
patch (78.46 KB, patch)
2012-03-08 21:52 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (78.47 KB, patch)
2012-03-08 21:58 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (78.50 KB, patch)
2012-03-08 22:25 PST, Andreas Gal :gal
cajbir.bugzilla: review+
Details | Diff | Review
patch (78.50 KB, patch)
2012-03-08 23:26 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (78.50 KB, patch)
2012-03-08 23:27 PST, Andreas Gal :gal
no flags Details | Diff | Review
Decode mp3 audio (1.06 KB, patch)
2012-03-08 23:27 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
only set audio/video format if we have corresponding tracks (565 bytes, patch)
2012-03-08 23:35 PST, Andreas Gal :gal
no flags Details | Diff | Review
Don't ReadVideo() if there's no video stream (487 bytes, patch)
2012-03-09 00:03 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
Fix memory leak (3.52 KB, patch)
2012-03-12 20:38 PDT, cajbir (:cajbir)
cpearce: review+
Details | Diff | Review
a patch adds hw video codec support on ics's b2g (7.74 KB, patch)
2012-04-23 23:19 PDT, Sotaro Ikeda
no flags Details | Diff | Review
temporary patch: no video frame copy rendering (160.69 KB, patch)
2012-05-11 01:41 PDT, Sotaro Ikeda
no flags Details | Diff | Review
temporary patch: no video frame copy rendering (157.80 KB, patch)
2012-05-13 19:13 PDT, Sotaro Ikeda
no flags Details | Diff | Review
temporary patch: no video frame copy rendering (158.84 KB, patch)
2012-05-13 22:51 PDT, Sotaro Ikeda
no flags Details | Diff | Review
temporary patch: no video frame copy rendering (172.59 KB, patch)
2012-05-15 01:24 PDT, Sotaro Ikeda
no flags Details | Diff | Review
temporary patch: no video frame copy rendering (174.36 KB, patch)
2012-05-15 19:15 PDT, Sotaro Ikeda
no flags Details | Diff | Review
Patch (83.99 KB, patch)
2012-05-15 19:35 PDT, Edwin Flores [:eflores] [:edwin]
roc: review+
Details | Diff | Review
temporary patch: no video frame copy rendering (174.36 KB, patch)
2012-05-15 19:36 PDT, Sotaro Ikeda
no flags Details | Diff | Review
Part 1: Configure/Toolkit changes (6.40 KB, patch)
2012-05-20 20:32 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
Part 2: Media plugin/Omx Plugin support for mp4/mp3 playback via libstagefright (65.27 KB, patch)
2012-05-20 20:35 PDT, cajbir (:cajbir)
cajbir.bugzilla: review+
Details | Diff | Review
Part 1: Configure/Toolkit changes (6.40 KB, patch)
2012-05-21 22:20 PDT, cajbir (:cajbir)
mh+mozilla: review+
Details | Diff | Review
Part 2: Media plugin/Omx Plugin support for mp4/mp3 playback via libstagefright (65.50 KB, patch)
2012-05-21 22:29 PDT, cajbir (:cajbir)
cajbir.bugzilla: review+
Details | Diff | Review
Part 2: Media plugin/Omx Plugin support for mp4/mp3 playback via libstagefright (65.57 KB, patch)
2012-05-22 19:53 PDT, cajbir (:cajbir)
cajbir.bugzilla: review+
Details | Diff | Review
temporary patch: no video frame copy rendering (174.54 KB, patch)
2012-05-23 00:44 PDT, Sotaro Ikeda
no flags Details | Diff | Review
Part 3: audio channel count initialized with wrong value - r=eflores (2.14 KB, patch)
2012-05-30 15:28 PDT, cajbir (:cajbir)
edwin: review+
Details | Diff | Review
Part 1: Configure/Toolkit changes (5.66 KB, patch)
2012-05-31 17:43 PDT, cajbir (:cajbir)
cajbir.bugzilla: review+
Details | Diff | Review
screenshot of no video (40.71 KB, image/png)
2012-06-04 17:29 PDT, Tony Chung [:tchung]
no flags Details

Description Andreas Gal :gal 2011-12-30 19:28:20 PST
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.
Comment 1 Andreas Gal :gal 2011-12-30 19:28:44 PST
Created attachment 585104 [details] [diff] [review]
patch
Comment 2 Andreas Gal :gal 2011-12-30 19:53:51 PST
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.
Comment 3 Andreas Gal :gal 2012-01-03 04:07:18 PST
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.
Comment 4 Andreas Gal :gal 2012-01-04 06:31:08 PST
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.
Comment 5 Andreas Gal :gal 2012-01-04 06:32:30 PST
(Note: the android source has to be present to build libomxplugin since we use header files that are not in the NDK)
Comment 6 Andreas Gal :gal 2012-01-05 21:45:34 PST
Making good progress here, new update soon.
Comment 7 Andreas Gal :gal 2012-01-06 02:11:34 PST
Created attachment 586367 [details] [diff] [review]
patch
Comment 8 Andreas Gal :gal 2012-01-12 22:13:09 PST
Created attachment 588313 [details] [diff] [review]
patch
Comment 9 Sotaro Ikeda 2012-01-29 18:20:57 PST
Created attachment 592577 [details] [diff] [review]
make patch to b2g's source code
Comment 10 Sotaro Ikeda 2012-01-29 18:30:57 PST
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 Sotaro Ikeda 2012-01-29 19:10:34 PST
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 Sotaro Ikeda 2012-01-30 21:58:40 PST
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.
Comment 13 Andreas Gal :gal 2012-02-11 04:40:13 PST
Created attachment 596301 [details] [diff] [review]
Part 1: gonk patch to fix some quirks and disable old drivers for akami
Comment 14 Andreas Gal :gal 2012-02-11 04:40:34 PST
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();
Comment 15 Andreas Gal :gal 2012-02-11 04:41:21 PST
Created attachment 596302 [details] [diff] [review]
Part 2: MPAPI and omx-plugin for B2G
Comment 16 Andreas Gal :gal 2012-02-11 04:41:54 PST
Created attachment 596303 [details] [diff] [review]
Part 3: enable omx-plugin in build config
Comment 17 Andreas Gal :gal 2012-02-11 04:42:39 PST
Created attachment 596304 [details]
test case for B2G, install as /data/local/homescreen.html along with sample.mp4
Comment 18 Andreas Gal :gal 2012-02-11 04:51:57 PST
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 Sotaro Ikeda 2012-02-12 22:14:02 PST
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".
Comment 20 Andreas Gal :gal 2012-02-12 22:36:09 PST
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?
Comment 21 Andreas Gal :gal 2012-02-13 23:09:24 PST
Created attachment 596932 [details] [diff] [review]
part 2: gecko patch
Comment 22 Sotaro Ikeda 2012-02-13 23:56:24 PST
(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?
Comment 23 Andreas Gal :gal 2012-02-13 23:58:17 PST
Strange. Sure, let me check again.
Comment 24 Andreas Gal :gal 2012-02-14 00:01:17 PST
Created attachment 596938 [details] [diff] [review]
part 2: gecko patch
Comment 25 Andreas Gal :gal 2012-02-14 00:03:06 PST
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 Sotaro Ikeda 2012-02-14 00:43:58 PST
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.
Comment 27 Andreas Gal :gal 2012-02-14 00:46:12 PST
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 Sotaro Ikeda 2012-02-14 20:32:42 PST
Created attachment 597288 [details] [diff] [review]
interdiff patch

interdiff patch to fix following problems on emulator
  - no video drawing
  - seek
Comment 29 Michael Vines [:m1] [:evilmachines] 2012-02-15 16:26:27 PST
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 Sotaro Ikeda 2012-02-15 17:10:49 PST
> 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 Sotaro Ikeda 2012-02-15 17:31:01 PST
> 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.
Comment 32 Michael Vines [:m1] [:evilmachines] 2012-02-15 19:39:19 PST
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 Sotaro Ikeda 2012-02-15 20:09:25 PST
> 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;
>        }
Comment 34 Michael Vines [:m1] [:evilmachines] 2012-02-15 20:24:57 PST
Cool, lets do that then.  The for(;;) is definitely better than a random number.
Comment 35 Andreas Gal :gal 2012-02-17 01:54:04 PST
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).
Comment 36 Andreas Gal :gal 2012-02-17 01:56:02 PST
I also removed Pin/Unpin for MPAPI. We can hide that in Gecko. Fewer things to get wrong in the plugin.
Comment 37 Andreas Gal :gal 2012-02-19 01:57:49 PST
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.
Comment 38 Andreas Gal :gal 2012-02-19 02:05:28 PST
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.
Comment 39 Andreas Gal :gal 2012-02-19 02:07:55 PST
Comment on attachment 598641 [details] [diff] [review]
Part 2: gecko patch

Bugzilla broken. I can't request review. Awesome. Will do via email.
Comment 40 Andreas Gal :gal 2012-02-19 02:13:59 PST
I landed part 1 on github. Only the gecko part remains now.
Comment 41 Andreas Gal :gal 2012-02-22 10:26:59 PST
Created attachment 599684 [details] [diff] [review]
Part 2: gecko patch
Comment 42 Andreas Gal :gal 2012-02-22 10:27:40 PST
Created attachment 599685 [details] [diff] [review]
Part 2: gecko patch
Comment 43 Andreas Gal :gal 2012-02-22 10:34:22 PST
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.
Comment 44 Andreas Gal :gal 2012-02-25 15:31:20 PST
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.
Comment 45 cajbir (:cajbir) 2012-02-26 19:45:12 PST
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.
Comment 46 Andreas Gal :gal 2012-03-08 21:52:56 PST
Created attachment 604305 [details] [diff] [review]
patch
Comment 47 Andreas Gal :gal 2012-03-08 21:58:43 PST
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.
Comment 48 Andreas Gal :gal 2012-03-08 22:25:14 PST
Created attachment 604312 [details] [diff] [review]
patch

Use new CheckedInt64 return value.
Comment 49 Andreas Gal :gal 2012-03-08 23:26:38 PST
Created attachment 604317 [details] [diff] [review]
patch

don't configure video if we have audio only and vice versa
Comment 50 Andreas Gal :gal 2012-03-08 23:27:34 PST
Created attachment 604318 [details] [diff] [review]
patch

don't try to configure audio/video if there is no track for it
Comment 51 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-08 23:27:43 PST
Created attachment 604319 [details] [diff] [review]
Decode mp3 audio

Fix for audio-only stream likely overlaps with just-posted patch.
Comment 52 Andreas Gal :gal 2012-03-08 23:35:27 PST
Created attachment 604322 [details] [diff] [review]
only set audio/video format if we have corresponding tracks
Comment 53 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-09 00:03:17 PST
Created attachment 604334 [details] [diff] [review]
Don't ReadVideo() if there's no video stream

This patch feels wrong but fixes another crasher.
Comment 54 Andreas Gal :gal 2012-03-09 00:05:47 PST
Ok thats wrong we shouldn't get there.
Comment 55 :Ms2ger 2012-03-12 12:25:47 PDT
Comment on attachment 604312 [details] [diff] [review]
patch

Clearly not in line with Mozilla policy.
Comment 56 cajbir (:cajbir) 2012-03-12 20:38:10 PDT
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 Ben Bucksch (:BenB) 2012-03-20 08:35:19 PDT
This is a DUP of bug 541286
Comment 58 Ben Bucksch (:BenB) 2012-03-20 08:54:39 PDT
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.
Comment 59 Henri Sivonen (:hsivonen) 2012-03-21 00:26:17 PDT
(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 Ben Bucksch (:BenB) 2012-03-21 02:37:03 PDT
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.)
Comment 61 Henri Sivonen (:hsivonen) 2012-03-21 02:55:24 PDT
(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 Ben Bucksch (:BenB) 2012-03-21 04:59:29 PDT
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.
Comment 63 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-17 18:50:49 PDT
doublec, do you own this now?  I don't think Andreas is working on this anymore.
Comment 64 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-17 19:48:41 PDT
doublec is on vacation, back in a couple of weeks.
Comment 65 Sotaro Ikeda 2012-04-23 23:19:38 PDT
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
Comment 66 cajbir (:cajbir) 2012-04-27 14:14:03 PDT
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].
Comment 67 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-27 15:15:59 PDT
This can land.
Comment 68 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-27 23:23:49 PDT
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.
Comment 69 Maire Reavy [:mreavy] (Plz ni?:mreavy) 2012-04-28 00:41:41 PDT
(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.
Comment 70 Andreas Gal :gal 2012-05-07 14:53:22 PDT
#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 Sotaro Ikeda 2012-05-11 01:41:13 PDT
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 Sotaro Ikeda 2012-05-11 01:45:19 PDT
by using attachment 623069 [details] [diff] [review] on my phone (omap dualcore, ics).

H.264 720P 24fps : spent about 20% cpu time during playback.
Comment 73 Randell Jesup [:jesup] 2012-05-11 07:04:24 PDT
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.
Comment 74 Sotaro Ikeda 2012-05-13 19:06:33 PDT
>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 Sotaro Ikeda 2012-05-13 19:13:23 PDT
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.
Comment 76 Sotaro Ikeda 2012-05-13 22:36:12 PDT
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 Sotaro Ikeda 2012-05-13 22:51:36 PDT
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;
//    }
Comment 78 Sotaro Ikeda 2012-05-15 01:24:13 PDT
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.
Comment 79 cajbir (:cajbir) 2012-05-15 14:05:51 PDT
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?
Comment 80 Andreas Gal :gal 2012-05-15 14:24:55 PDT
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.
Comment 81 Chris Pearce (:cpearce) 2012-05-15 14:36:21 PDT
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.
Comment 82 Andreas Gal :gal 2012-05-15 15:13:20 PDT
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 Sotaro Ikeda 2012-05-15 17:17:44 PDT
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 Sotaro Ikeda 2012-05-15 17:43:32 PDT
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 Sotaro Ikeda 2012-05-15 19:15:33 PDT
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
Comment 86 Edwin Flores [:eflores] [:edwin] 2012-05-15 19:35:41 PDT
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.
Comment 87 Sotaro Ikeda 2012-05-15 19:36:25 PDT
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();
Comment 88 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-15 20:06:24 PDT
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.
Comment 89 cajbir (:cajbir) 2012-05-15 20:57:25 PDT
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.
Comment 90 Chris Pearce (:cpearce) 2012-05-16 19:53:01 PDT
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.
Comment 91 cajbir (:cajbir) 2012-05-20 20:32:33 PDT
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)
Comment 92 cajbir (:cajbir) 2012-05-20 20:35:57 PDT
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.
Comment 93 cajbir (:cajbir) 2012-05-21 21:58:07 PDT
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.
Comment 94 cajbir (:cajbir) 2012-05-21 22:20:52 PDT
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)
Comment 95 cajbir (:cajbir) 2012-05-21 22:29:14 PDT
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).
Comment 96 cajbir (:cajbir) 2012-05-22 19:53:42 PDT
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.
Comment 97 Sotaro Ikeda 2012-05-23 00:44:21 PDT
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
Comment 98 dclarke@mozilla.com [:onecyrenus] 2012-05-25 17:10:46 PDT
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.
Comment 99 cajbir (:cajbir) 2012-05-26 22:56:30 PDT
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.
Comment 100 cajbir (:cajbir) 2012-05-28 21:25:05 PDT
Comment on attachment 625897 [details] [diff] [review]
Part 1: Configure/Toolkit changes

Requesting review from khuey in case ted is swamped.
Comment 101 dclarke@mozilla.com [:onecyrenus] 2012-05-29 17:33:38 PDT
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.
Comment 102 cajbir (:cajbir) 2012-05-29 17:46:03 PDT
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.
Comment 103 cajbir (:cajbir) 2012-05-30 15:28:04 PDT
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.
Comment 104 cajbir (:cajbir) 2012-05-30 18:17:09 PDT
Comment on attachment 625897 [details] [diff] [review]
Part 1: Configure/Toolkit changes

Removed :khuey since he's on vacation.
Comment 105 Mike Hommey [:glandium] 2012-05-31 00:22:51 PDT
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).
Comment 106 Mike Hommey [:glandium] 2012-05-31 00:27:51 PDT
(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.
Comment 107 cajbir (:cajbir) 2012-05-31 14:40:15 PDT
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.
Comment 108 cajbir (:cajbir) 2012-05-31 17:43:42 PDT
Created attachment 629012 [details] [diff] [review]
Part 1: Configure/Toolkit changes

Address review comments, carrying forward r+
Comment 111 dclarke@mozilla.com [:onecyrenus] 2012-06-04 16:51:10 PDT
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 ?
Comment 112 Andreas Gal :gal 2012-06-04 16:53:05 PDT
Chris whats up here? Our silicon vendor friends were complaining about horribly low frame rates too (5FPS etc).
Comment 113 cajbir (:cajbir) 2012-06-04 16:54:41 PDT
(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.
Comment 114 cajbir (:cajbir) 2012-06-04 17:01:27 PDT
(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
Comment 115 Tony Chung [:tchung] 2012-06-04 17:29:32 PDT
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?
Comment 116 Tony Chung [:tchung] 2012-06-04 17:37:34 PDT
(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.
Comment 117 cajbir (:cajbir) 2012-06-04 17:38:47 PDT
(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?
Comment 118 dclarke@mozilla.com [:onecyrenus] 2012-06-04 17:57:45 PDT
Confirmed audio playback is slow, also the controls for volume / rewind didn't seem to respond. Is this expected ?
Comment 119 cajbir (:cajbir) 2012-06-04 18:02:24 PDT
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?
Comment 120 Maire Reavy [:mreavy] (Plz ni?:mreavy) 2012-06-07 13:19:38 PDT
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!
Comment 121 cajbir (:cajbir) 2012-06-07 14:55:49 PDT
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.
Comment 122 dclarke@mozilla.com [:onecyrenus] 2012-06-07 16:34:20 PDT
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.
Comment 123 Maire Reavy [:mreavy] (Plz ni?:mreavy) 2012-06-07 16:59:25 PDT
(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!
Comment 124 dclarke@mozilla.com [:onecyrenus] 2012-06-07 17:11:19 PDT
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.
Comment 125 Maire Reavy [:mreavy] (Plz ni?:mreavy) 2012-06-07 17:27:33 PDT
(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!
Comment 126 cajbir (:cajbir) 2012-06-07 19:23:15 PDT
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.
Comment 127 Jason Smith [:jsmith] 2012-06-26 07:22:09 PDT
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.
Comment 128 dclarke@mozilla.com [:onecyrenus] 2012-06-26 12:33:49 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.