+++ This bug was initially created as a clone of Bug #714408 +++ Bug 714408 gives us playback by reading back frames from the decoder, but we need to use the non-copying playback path for low-end phones. Basically, we get a handle to a surface that we can later bind to a texture. I believe attachment 626354 [details] [diff] [review] is the start of that work. Thanks Sotaro-san!

I created attachment 626354 [details] [diff] [review] for just to show a possibility of zoro-copy video rendering with libstagefright. The patch is created just as temporary. Therefore, the patch has a lot of problems need to fix. For example, I created SurfaceTextureGonk and SurfaceTextureClientGonk classes from android source code. This need to be avoided. bug 757341 fixes the problem better way and is more aligned to gecko's architecture.

people.mozilla.com/~dclarke/b.html <--- I have a 720p sample. I think 720p on mobile is fairly standard nowadays. I am unable to play this content at a decent frame rate.

This patch, with its new GONK_IO_SURFACE Image type, seems like the right idea. The integration with the media stack needs to be refactored though. We don't want + void *mMediaBuffer; + void *mNativeWindow; + void *mSurfaceTexture; in cross-platform VideoData. It should be enough to create the GonkIOSurfaceImage with that data and store it in the existing VideoData.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #3) > This patch, with its new GONK_IO_SURFACE Image type, seems like the right > idea. > > The integration with the media stack needs to be refactored though. We don't > want > + void *mMediaBuffer; > + void *mNativeWindow; > + void *mSurfaceTexture; > in cross-platform VideoData. It should be enough to create the > GonkIOSurfaceImage with that data and store it in the existing VideoData. I'm working on refactoring that part in bug 757341, will have review able patch soon.

OK. We'll wait for you to finish the work in bug 757341.

Robert, I've been doing some work on this bug to reuse the GonkIOSurfaceImage implementation in bug 757341. I've gotten it to the point where it renders the "bucky bunny" test video albeit with some flicker. I'll be in the work week on June 25th so hopefully I'll be able to sync up with any mozilla dev working on this.

Edwin Flores is there, please locate him and work with him on this.

Just putting this up so we can communicate over bugzilla.

Comment on attachment 640440 [details] [diff] [review] Do not use, land, or even really read. Review of attachment 640440 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContextProviderEGL.cpp @@ +418,5 @@ > + EGL_NO_CONTEXT, > + EGL_NATIVE_BUFFER_ANDROID, > + buffer, attrs); > + sEGLLibrary.fImageTargetTexture2DOES(GL_TEXTURE_EXTERNAL_OES, image); > + fBindTexture(GL_TEXTURE_EXTERNAL_OES, texture); Repeating the two calls above here (fImageTargetTexture2DOES and fBindTexture) fixed the flickering issue on the ICS akami. It may be a bug in the graphic library. I haven't debugged further on that front yet.

I read the attachment 640440 [details] [diff] [review]. BindExternalBuffer() in attachment 640440 [details] [diff] [review] calls following functions sequentially. - sEGLLibrary.fCreateImage(EGL_DISPLAY(), - sEGLLibrary.fImageTargetTexture2DOES(GL_TEXTURE_EXTERNAL_OES, image); - fBindTexture(GL_TEXTURE_EXTERNAL_OES, texture); - sEGLLibrary.fDestroyImage(EGL_DISPLAY(), image); In the function, EGL image is created and then soon destroyed. This happens every Video frame rendering. I am not sure whether this way of rendering is safe for production. It is very different from how android uses EGLImages. In android case, EGLImage is re-created only when related GraphicBuffer is recreated. In android case GraphicBuffer is created and managed by server side and rendered by client-side. But GonkNativeWindow is created by client side and push the GraphicBuffer to server-side(Layer). It seems that this difference request EGLImage creation/destruction in every Video frame rendering.

Closer to final product. Applies cleanly to demo repo. Waiting on ICS Otoro image to test.

Hey Edwin, how are things looking? Running into any issues?

Closer. Trying to track down a catastrophic kernel panic (I suspect gralloc-related) at the moment. Will try to sort out on the flight tomorrow.

Rebased against m-c; removed OMX-specific GonkNativeWindow. Trying to track down a suspected use-after-free bug; would be nice to get any insight on that. Still not quite landable -- should be okay after a quick cleanup and once the crash above is fixed. ...for software decoders, that is. Blocking on Qualcomm for kernel fix as far as hardware support goes.

Comment on attachment 647454 [details] [diff] [review] WIP Review of attachment 647454 [details] [diff] [review]: ----------------------------------------------------------------- In most places: s/NULL/nullptr Use static_cast where possible instead of reinterpret_cast. Lifetime of some of the objects as mentioned in review comments, isn't well documented. ::: content/media/nsBuiltinDecoderReader.cpp @@ +39,5 @@ > #define SEEK_LOG(type, msg) > #endif > > +#include <android/log.h> > +#define logcat(x...) __android_log_print(ANDROID_LOG_INFO, "Reader", x) Needs #ifdef for GONK/ANDROID @@ +231,5 @@ > + aKeyframe, > + aTimecode, > + aInfo.mDisplay)); > + > + Image::Format format = Image::GONK_IO_SURFACE; Is this Gonk specific? If so, needs a path for non-gonk and a way to build on non-gonk platforms. ::: content/media/omx/OmxDecoder.cpp @@ +86,5 @@ > +} > + > +ssize_t MediaStreamSource::readAt(off64_t offset, void *data, size_t size) > +{ > + char *ptr = reinterpret_cast<char *>(data); Use static_cast @@ +156,5 @@ > + > +class AutoStopMediaSource { > + sp<MediaSource> mMediaSource; > +public: > + AutoStopMediaSource(sp<MediaSource> aMediaSource) : mMediaSource(aMediaSource) { Make 'aMediaSource' and constant reference to avoid unnecessary copying: AutoStopMediaSource(const sp<MediaSource>& aMediaSource) @@ +232,5 @@ > + //mPluginHost->SetPlaybackReadMode(mDecoder); > + > + int64_t totalDurationUs = 0; > + > + mNativeWindow = new GonkNativeWindow(); How is the lifetime of this managed? Should mNativeWindow be an sp? @@ +458,5 @@ > +} > + > +bool OmxDecoder::ToAudioFrame(AudioFrame *aFrame, int64_t aTimeUs, void *aData, size_t aDataOffset, size_t aSize, int32_t aAudioChannels, int32_t aAudioSampleRate) > +{ > + aFrame->Set(aTimeUs, reinterpret_cast<char *>(aData) + aDataOffset, aSize, aAudioChannels, aAudioSampleRate); static_cast @@ +473,5 @@ > + MediaBuffer *throwaway; > + > + MediaSource::ReadOptions options; > + options.setSeekTo(aSeekTimeUs, MediaSource::ReadOptions::SEEK_PREVIOUS_SYNC); > + status_t err = mVideoSource->read(&throwaway, &options); throwaway needs to be released if the read succeeds I think. @@ +532,5 @@ > + //blah = (blah + 10000) % 90000; > + //memset(innerData, 0xffffffff, 90000 + blah); > + //inner->unlock(); > + SurfaceDescriptor* descriptor = > + mNativeWindow->getSurfaceDescriptorFromBuffer(mVideoBuffer->graphicBuffer().get()); This appears to return an internal pointer from inside the graphic buffer. How is it determined the the video buffer and/or graphic buffer doesn't invalidate this pointer. Can it be a counted pointer somehow? @@ +537,5 @@ > + if (!descriptor) { > + logcat("SurfaceDescriptor is NULL"); > + return false; > + } > + aFrame->mGraphicBuffer = new mozilla::layers::VideoGraphicBuffer(mVideoBuffer, descriptor); mGraphicBuffer is a raw pointer. So how is the lifetime of this managed? Should it be a nsRefPtr? It doesn't appear to be deleted anywhere. @@ +555,5 @@ > + unreadable, > + timeUs, > + keyFrame); > + > + char *data = reinterpret_cast<char *>(mVideoBuffer->data()) + mVideoBuffer->range_offset(); static_cast @@ +662,5 @@ > + > +static void DestroyDecoder(Decoder *aDecoder) > +{ > + if (aDecoder->mPrivate) > + delete reinterpret_cast<OmxDecoder *>(aDecoder->mPrivate); Use the 'cast' function here? ::: content/media/omx/OmxDecoder.h @@ +85,5 @@ > + MediaBuffer *mVideoBuffer; > + VideoFrame mVideoFrame; > + MediaBuffer *mAudioBuffer; > + AudioFrame mAudioFrame; > + And comments on how the lifetime of all the raw pointers are managed. Can some form of counter pointer be used instead?

In that stack trace, it looks like StateMachineTracker::Instance() returning a bogus object? Have you tried inspecting StateMachineTracker::sInstance to see if it's valid? Try adding some logging to StateMachineTracker's constructor and destructor to see if it's not being initialized or being destroyed too early?

aData in RenderVideoFrame seems to be bogus. I don't know if that matters or helps track things down.

Rebase

Tiny rebase

No longer crashes on startup when using software decoders. New crash a bit into playback.

Got rid of crashiness

How are we doing here?

Comment on attachment 654094 [details] [diff] [review] WIP Review of attachment 654094 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/nsBuiltinDecoderReader.cpp @@ +228,5 @@ > + aKeyframe, > + aTimecode, > + aInfo.mDisplay)); > + > + Image::Format format = Image::GONK_IO_SURFACE; Image::Format was renamed to ImageFormat. You may need to include ImageTypes.h

Sprayed with bitrot-b-gone, left for an hour and rinsed off.

Edwin, how is this coming along? If you comment out this http://mxr.mozilla.org/mozilla-central/source/ipc/glue/GeckoChildProcessHost.cpp#442 so that privs always ends up as SAME_PRIVILEGES_AS_PARENT, does the hw decoder work in update7?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #29) > > so that privs always ends up as SAME_PRIVILEGES_AS_PARENT, does the hw > decoder work in update7? Hopefully Edwin will comment on how his work is going, but in the meantime if I do this then the hardware decoder is instantiated with the existing media/omx-plugin code. Without it it does not get instantiated (with kernel update7). Unfortunately it crashes later on with a SIGSEGV in plugin-container.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #29) > If you comment out this > > http://mxr.mozilla.org/mozilla-central/source/ipc/glue/GeckoChildProcessHost. > cpp#442 > > so that privs always ends up as SAME_PRIVILEGES_AS_PARENT, does the hw > decoder work in update7? Seeing the same as Chris D was. Hardware decoder is instantiated, but I'm now getting a sadface from ImageBridgeChild as (1) it doesn't like to be used outside of the ImageBridgeChild thread; and (2) sImageBridgeChildThread is null. This happens when trying to allocate the first frame. Reproducible using any software decoder due to the recent change adding GRALLOC_PLANAR_YCBCR images. Not seeing this from the camera -- OOP related? I'm not sure how relevant this is, as I still only see black, but logcat suggests it's allocating gralloc buffers fine. Will attach backtrace.

You need to apply https://bugzilla.mozilla.org/attachment.cgi?id=655348 . Landing today.

How are things looking with latest inbound and the hack above?

Works reasonably well with 4-week-old gecko and kernelupdate7. Updating either gecko or kernel breaks HW decoding. Updating kernel, it seems to run out of pmem space. Trying to track down regression in gecko right now -- all I know so far is that the OMX decoder fails when calling allocateNode with OMX_ErrorInsufficientResources return code. Known issue with software decoder giving the wrong aspect ratio. Also, crashes when using the GRALLOC_PLANAR_YCBCR image type (see bug 790322). Still needs cleaning up.

With old gecko, what does |adb shell ps| say when you're playing video? What does it say with new gecko?

Also, should add that we dropped the pmem allocation to the video decoder in kernel8. We should see why it's using an unexpected amount of pmem, and whether we have a driver / library bug to fix or need to bump the allocation back up (sadmaking!).

(In reply to Chris Jones [:cjones] [:warhammer] from comment #36) > With old gecko, what does |adb shell ps| say when you're playing video? > What does it say with new gecko? Can't seem to get old gecko working now either; must have **** something up when rebasing. Falling into the "we have no source for this" hole for libOmxH264Dec.so trying to debug it.

Workaround in bug 791164 works with kernelupdate7; investigating kernelupdate8 issue now.

Edwin, please make sure you talk to our silicon vendor friends. They can look at the source.

Edwin, Some of the code in OmxDecoder.cpp and nsMediaOmxReader.cpp is out of date with respect to bug fixes that have been applied to the media/omx-plugin code. You'll need to add a fix equivalent to bug 783927 to avoid early stoppage of some video playback due to libstagefright returning zero byte reads which is being treated as EOF. You'll also need to adjust the INFO_FORMAT_CHANGED handling so it recursively calls (or loops) back to ReadAudio/ReadVideo, otherwise playback can again stop early in the video. See OmxPlugin.cpp's ReadAudio/ReadVideo for how that was changed to handle it.

This patch works for video playback in the video app with the following: 1) Workaround from bug 791164 applied 2) Thumbnail generation disabled in the Gaia video app. (2) causes an assertion/crash for some reason with H.264 videos. I'll raise a bug shortly.

Bug 791912 is for the assertion mentioned in comment 44.

Looking at the buffer allocation failure log. It's trying to allocate 14 output buffers. That is *way* too many. Couple that with the fact that the new kernel has less PMEM available and you run out of memory.

Any idea why we are allocating so many?

Not yet. I'm investigating on the HW decoder side. I'll update ASAP

nsBuiltinDecoderStateMachine::HaveEnoughDecodedVideo tries to decode up to 10 video frames (AMPLE_VIDEO_FRAMES) for buffering purposes. We can tune this. We should probably lower it substantially for hardware decoders.

Turns out 14 buffers is the correct count. This number was reached by analyzing the performance of a wide range of >=QVGA to <=FWVGA clips. That was the reason for the original size of the PMEM region used by hardware video codecs. I can find more details on this if anyone is interested.

(In reply to Diego Wilson from comment #50) > Turns out 14 buffers is the correct count. This number was reached by > analyzing the performance of a wide range of >=QVGA to <=FWVGA clips. That > was the reason for the original size of the PMEM region used by hardware > video codecs. I can find more details on this if anyone is interested. I would like to hear more details :)

IIRC, current qualcomm's platform has two pmems(/dev/pmem and /dev/pmem_adsp). - "/dev/pmem_adsp" is used for hw codec , camera and GRALLOC_USAGE_EXTERNAL_DISP. - "/dev/pmem" is used for other normal GraphcBuffer. Is it correct also for otoro? If so, hw codec's buffer starvation does not affect normal GraphicBuffer usages like from ThebesLayer, I assume. Or I might be wrong.

You'll need to split the patch up a bit for separate review unfortunately: 1) dom/camera stuff to be reviewed by whoever owns that. Probably cjones. 2) Build stuff (configure.in, toolkit/*, layout/*) 3) Media stuff (content/media/*, content/html/content/src/nsHTMLMedia*) Have you tested to see if it builds in non-b2g builds? Tryserver? I'll review the (3) stuff later today.

We have a MOZ_OMX_PLUGIN and a MOZ_OMX which can be a bit confusing. I wonder if we should make the latter MOZ_OMX_BUILTIN (or similar) with --enable-omx-builtin. And make it that omx-plugin and omx-builtin can't both be defined.

Comment on attachment 663869 [details] [diff] [review] Part 3 - Support for zero-copy OMX hardware decoding Review of attachment 663869 [details] [diff] [review]: ----------------------------------------------------------------- r+ with minor things fixed. At some point we should look at how we can merge/share code with media/omx-plugin and content/media/plugin. ::: content/html/content/src/nsHTMLMediaElement.cpp @@ +2267,5 @@ > } > #endif > +#ifdef MOZ_OMX > + if (IsH264Type(nsDependentCString(aMIMEType))) { > + return CANPLAY_YES; This should be CANPLAY_MAYBE. ::: content/media/nsBuiltinDecoderStateMachine.cpp @@ +66,5 @@ > +#ifdef MOZ_WIDGET_GONK > +// On B2G this is decided by a similar constant in the OMX decoder. If this > +// number is higher than the OMX equivalent then gecko will think it is > +// chronically starved of video frames. > +static const uint32_t AMPLE_VIDEO_FRAMES = 5; Where in the OMX decoder is this defined? Maybe mention that here in the comment. Is it possible to do a STATIC_ASSERT to ensure the value is less than the OMX version? ::: content/media/omx/OmxDecoder.cpp @@ +25,5 @@ > +#include <OMX_Core.h> > +#include <OMX_Index.h> > +#include <OMX_IVCommon.h> > +#include <OMX_Component.h> > + These include files can be trimmed. A smaller subset like the following works: #include "base/basictypes.h" #include <stagefright/DataSource.h> #include <stagefright/MediaExtractor.h> #include <stagefright/MetaData.h> #include <stagefright/OMXCodec.h> #include <OMX.h> @@ +443,5 @@ > + case OMX_QCOM_COLOR_FormatYVU420SemiPlanar: > + SemiPlanarYVU420Frame(aFrame, aTimeUs, aData, aSize, aKeyFrame); > + break; > + default: > + return false; Add a LOG here about unknown format with the mVideoColorFormat included. @@ +572,5 @@ > + } > + else if (err == INFO_FORMAT_CHANGED && !SetAudioFormat()) { > + // If the format changed, update our cached info. > + if (!SetAudioFormat()) { > + return false; SetAudioFormat is called twice here if it fails the first time. Probably remove it from the 'if' condition. ::: content/media/omx/nsMediaOmxReader.cpp @@ +17,5 @@ > + > +nsMediaOmxReader::nsMediaOmxReader(nsBuiltinDecoder *aDecoder) : > + nsBuiltinDecoderReader(aDecoder), > + mOmxDecoder(nullptr), > + mVideoSeekTimeUs(-1), mHasAudio and mHasVideo need to be initialized to false here. ::: content/media/omx/nsMediaOmxReader.h @@ +17,5 @@ > +class nsMediaOmxReader : public nsBuiltinDecoderReader > +{ > + nsCString mType; > + android::OmxDecoder *mOmxDecoder; > + //MPAPI::Decoder *mPlugin; Remove this line. @@ +19,5 @@ > + nsCString mType; > + android::OmxDecoder *mOmxDecoder; > + //MPAPI::Decoder *mPlugin; > + PRBool mHasAudio; > + PRBool mHasVideo; Use bool, not PRBool.

Addressed review comments; carry over r=doublec

Comment on attachment 663868 [details] [diff] [review] Part 2 - GonkNativeWindow and layers changes Review of attachment 663868 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/camera/GonkNativeWindow.cpp @@ +446,5 @@ > { > switch (operation) { > + case NATIVE_WINDOW_SET_BUFFERS_SIZE: > + case NATIVE_WINDOW_SET_SCALING_MODE: > + case NATIVE_WINDOW_SET_CROP: Do we handle the cropping and scaling or just ignore it? ::: gfx/layers/opengl/ImageLayerOGL.cpp @@ +454,5 @@ > GetVisibleRegion().GetBounds(), > nsIntSize(ioImage->GetSize().width, > ioImage->GetSize().height)); > + > + mImage = image; Do we still need to do this?

(In reply to Chris Double (:doublec) from comment #59) > > + if (IsH264Type(nsDependentCString(aMIMEType))) { > > + return CANPLAY_YES; > > This should be CANPLAY_MAYBE. Why only maybe? See bug 760140 comment 7 and bug 760140 comment 9. I think canPlayType("video/mp4") should say "maybe" and video/mp4; codecs="avc1.42E01E, mp4a.40.2" should say "probably" for consistency with other H.264-supporting browsers and to get the right score on html5test.com.

Edwin, what is the difference between content/media/omx/OmxDecoder.cpp and media/omx-plugin/OmxPlugin.cpp (currently used on Android and B2G)?

Comment on attachment 663867 [details] [diff] [review] Part 1 - Build Changes Is there any reason not to enable this by default for b2g?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #64) > Comment on attachment 663867 [details] [diff] [review] > Part 1 - Build Changes > > Is there any reason not to enable this by default for b2g? More precisely, I mean can we use gonk <=> enable_omx, instead of adding a configure option?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #64) > > Is there any reason not to enable this by default for b2g? That's a good idea and I've been leaving the decision whether to enable it by default immediately up to the b2g team. If you're fine with it, yes we should do it.

(In reply to Chris Peterson (:cpeterson) from comment #63) > Edwin, what is the difference between content/media/omx/OmxDecoder.cpp and > media/omx-plugin/OmxPlugin.cpp (currently used on Android and B2G)? It is very similar and sometime post-landing we'll refactor the code so implementations are shared.

Let's do it. Let's not change configure.in unless we absolutely need to ;).

(In reply to Henri Sivonen (:hsivonen) from comment #62) > > Why only maybe? Since we don't enumerate the formats that stagefright actually supports we don't know 100% that it supports H.264. Per spec: "Implementors are encouraged to return "maybe" unless the type can be confidently established as being supported or not." > I think canPlayType("video/mp4") should say "maybe" and video/mp4; > codecs="avc1.42E01E, mp4a.40.2" should say "probably" for consistency with > other H.264-supporting browsers and to get the right score on html5test.com. Please raise a bug for this.

(In reply to Chris Double (:doublec) from comment #69) > > I think canPlayType("video/mp4") should say "maybe" and video/mp4; > > codecs="avc1.42E01E, mp4a.40.2" should say "probably" for consistency with > > other H.264-supporting browsers and to get the right score on html5test.com. > > Please raise a bug for this. Change of plans. Edwin is going to do what gstreamer does here to get the detection working.

(In reply to Kan-Ru Chen [:kanru] from comment #61) > Do we handle the cropping and scaling or just ignore it? Ignoring it. Doesn't seem to be causing any problems with the videos we've tested so far. > ::: gfx/layers/opengl/ImageLayerOGL.cpp > > + > > + mImage = image; > Do we still need to do this? Doesn't look like it. Updated patch to come.

Minor change to enable OMX decoder by default in Gonk builds. Carry over r=doublec

Comment on attachment 664342 [details] [diff] [review] Part 1 rev 2 - Build changes r=me if it builds! ;)

Comment on attachment 664343 [details] [diff] [review] Part 2 rev 2 - GonkNativeWindow and layers changes r=me except for the change to ImageLayerOGL. roc, can you please review the one hunk at the end of the file?

Comment on attachment 664343 [details] [diff] [review] Part 2 rev 2 - GonkNativeWindow and layers changes Review of attachment 664343 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/opengl/ImageLayerOGL.cpp @@ +992,5 @@ > > gl()->ApplyFilterToBoundTexture(LOCAL_GL_TEXTURE_EXTERNAL, mFilter); > > program->Activate(); > + program->SetLayerQuadRect(GetVisibleRegion().GetBounds()); I don't think this change is correct. Can someone explain it?

Using the visible region bounds looks like the same kind of bug that led to bug 778029.

Edwin, you might want to make media.plugins.enabled be false in b2g/app/b2g.js since the omx stuff supercedes it.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #77) > Comment on attachment 664343 [details] [diff] [review] > Part 2 rev 2 - GonkNativeWindow and layers changes > > Review of attachment 664343 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/opengl/ImageLayerOGL.cpp > @@ +992,5 @@ > > > > gl()->ApplyFilterToBoundTexture(LOCAL_GL_TEXTURE_EXTERNAL, mFilter); > > > > program->Activate(); > > + program->SetLayerQuadRect(GetVisibleRegion().GetBounds()); > > I don't think this change is correct. Can someone explain it? That was the source of the scaling problem I was having -- the layer rect would be made the size of the whole image, but BindAndDrawQuadWithTextureRect was being passed the visible region for the texture coordinates. It was either this or passing BindAndDrawQuadWithTextureRect the whole image size. Doesn't matter either way.

(In reply to Edwin Flores [:eflores] [:edwin] from comment #80) > That was the source of the scaling problem I was having -- the layer rect > would be made the size of the whole image, but > BindAndDrawQuadWithTextureRect was being passed the visible region for the > texture coordinates. It was either this or passing > BindAndDrawQuadWithTextureRect the whole image size. Doesn't matter either > way. Definitely pass the whole image size to BindAndDrawQuadWithTextureRect.

You basically should not need to use the image's visible region during compositing. The only way we might want to use it is as an optimization to reduce the area composited, if we really know what we're doing and we care about improving performance when some of the image is not visible (and we can show that restricting compositing to the visible region actually does improve performance). I don't think any of those are the case here :-).

A recent change to the Gaia video app breaks error handling when thumbnails fail to generate with this patch applied, resulting in the video app not working. See bug 794055.

IIRC, When ANativeWindow is not used by OMXCodec, OMX color format ID is used by OMXCodec. And when ANativeWindow is used by OMXCodec, OMXCodec changed to use android pixel format. But if OMXCode use android pixcel format, OEM proprietary format ID seems not unique. This could affect to color conversion by software. Android framework do not need to identify the OEM color format and have only SurfaceTexture::isExternalFormat() function. It just recognize if a clolor format is external. http://androidxref.com/4.0.4/xref/frameworks/base/libs/gui/SurfaceTexture.cpp#849 The color format change is made by calling OMXCodec::initNativeWindow() and end up to set following attribute of OMX IL components. 　- "OMX.google.android.index.enableAndroidNativeBuffers" There is followign comment within initNativeWindow() // Enable use of a GraphicBuffer as the output for this node. This must // happen before getting the IndexParamPortDefinition parameter because it // will affect the pixel format that the node reports. http://androidxref.com/4.0.4/xref/frameworks/base/media/libstagefright/OMXCodec.cpp#4385 http://androidxref.com/4.0.4/xref/frameworks/base/media/libstagefright/omx/OMXNodeInstance.cpp#277 N.B. Even when ANativeWindow is set to OMXCode, it might be ignored by OMXCodec within OMXCodec::OMXCodec() (software codec case) http://androidxref.com/4.0.4/xref/frameworks/base/media/libstagefright/OMXCodec.cpp#1505

Pref off media plugins. Carry over r+

Edwin, Something like his patch is needed to fix a compiler warning and a compile error in DEBUG builds.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f7a7e009aa9 https://hg.mozilla.org/integration/mozilla-inbound/rev/3876b8007889 https://hg.mozilla.org/integration/mozilla-inbound/rev/69d3a8308851 https://hg.mozilla.org/integration/mozilla-inbound/rev/18becb73b906

https://hg.mozilla.org/mozilla-central/rev/6f7a7e009aa9 https://hg.mozilla.org/mozilla-central/rev/3876b8007889 https://hg.mozilla.org/mozilla-central/rev/69d3a8308851

Comment on attachment 664770 [details] [diff] [review] Part 1 rev 3 - Build changes Review of attachment 664770 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/build/Makefile.in @@ +192,5 @@ > > +ifeq (gonk,$(MOZ_WIDGET_TOOLKIT)) > +INCLUDES += \ > + -I$(srcdir)/../../base/src \ > + -I$(srcdir)/../../html/content/src \ Really, -I mozilla-central/html/content/src? I don't think I've seen that folder before.