Closed Bug 1199809 Opened 7 years ago Closed 6 years ago

Using FF/REW button on video app displays black screen instead of the video frame

Categories

(Core :: Audio/Video: Playback, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

()

VERIFIED FIXED
mozilla45
blocking-b2g 2.5+
Tracking Status
firefox45 --- fixed
b2g-v2.5 --- verified
b2g-master --- verified

People

(Reporter: njpark, Assigned: jhlin)

References

Details

(Keywords: regression, Whiteboard: fbimage)

Attachments

(6 files, 17 obsolete files)

2.05 KB, patch
jhlin
: review+
Details | Diff | Splinter Review
1.20 KB, patch
jhlin
: review+
Details | Diff | Splinter Review
1.40 KB, patch
jhlin
: review+
Details | Diff | Splinter Review
3.70 KB, patch
jhlin
: review+
Details | Diff | Splinter Review
12.77 KB, patch
jhlin
: review+
Details | Diff | Splinter Review
20.98 KB, patch
jhlin
: review+
Details | Diff | Splinter Review
STR:
Open Video app, select a video file (which is longer than 10 seconds)
While playing, pause it and use the FF/REW button to move the tile slider
Expected:
Slider is moved, and the correct video frame is shown
Actual:
Displays black screen

Can be reproduced in both aries and flame device
Version info:

Flame:
Build ID               20150828030207
Gaia Revision          b69c16798ddd7154207f56d983721a327522f5d1
Gaia Date              2015-08-27 23:43:56
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/87e23922be375985d0b1906ed5ba5f095f323a38
Gecko Version          43.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150828.063135
Firmware Date          Fri Aug 28 06:31:47 EDT 2015
Bootloader             L1TC000118D0

Aries:
Build ID               20150828140025
Gaia Revision          fa15462b29258fdec8329bfc367e590022dbc9e5
Gaia Date              2015-08-28 09:59:50
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/008d4d76f387b722fbee151e1c9e1501482054e5
Gecko Version          43.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20150828.132232
Firmware Date          Fri Aug 28 13:22:39 UTC 2015
Bootloader             s1
[Blocking Requested - why for this release]:
functional regression
blocking-b2g: --- → 2.5?
Keywords: regression
Whiteboard: fbimage
Video link:

http://youtu.be/m7rU_nuh7xw
QA Whiteboard: [QAnalyst-Triage+]
QA Contact: mshuman
This issue appears to be caused by either:
Bug 1195625 - Use correct TaskQueue in SharedDecoderManager and H264Converter promise.
OR
Bug 1179667 - Use MediaPromise for Gonk PlatformDecodeModule Init()

Mozilla-inbound Regression Window

Last Working 
Environmental Variables:
Device: Flame 2.5
BuildID: 20150818065331
Gaia: d9d99f32762975a370f1abd34a3512bd6fe29111
Gecko: 9a5c40d6f4b36d953e128155b9845e7bdcdcae85
Version: 43.0a1 (2.5)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

First Broken 
Environmental Variables:
Device: Flame 2.5
BuildID: 20150818071235
Gaia: d9d99f32762975a370f1abd34a3512bd6fe29111
Gecko: 560e0e0e3e8b5448fa91d36d4c9a9670ac5a0343
Version: 43.0a1 (2.5)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

Last Working gaia / First Broken gecko - Issue DOES reproduce
Gaia: d9d99f32762975a370f1abd34a3512bd6fe29111
Gecko: 560e0e0e3e8b5448fa91d36d4c9a9670ac5a0343

First Broken gaia / Last Working gecko - Issue does NOT reproduce
Gaia: d9d99f32762975a370f1abd34a3512bd6fe29111
Gecko: 9a5c40d6f4b36d953e128155b9845e7bdcdcae85

Gecko Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9a5c40d6f4b36d953e128155b9845e7bdcdcae85&tochange=560e0e0e3e8b5448fa91d36d4c9a9670ac5a0343
Blocks: 1195625
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(jmercado)
Alfredo this issue seems to have been caused by the changes for either bug 1179667 or bug 1195625.  Can you please take a look?
Blocks: 1179667
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmercado) → needinfo?(ayang)
I'll check it. Thanks for identifying the commits that cause the issue.
Assignee: nobody → jolin
Flags: needinfo?(ayang)
Component: Gaia::Video → Audio/Video: Playback
Product: Firefox OS → Core
Regression, Blocking for 2.5 with P2 priority.
blocking-b2g: 2.5? → 2.5+
Priority: -- → P2
Blocks: 1159343
Duplicate of this bug: 1206654
Turns out what Alfredo landed for bug 1179667 exposes a issue in the old code: before the patch, there's a bug that native window is create AFTER codec configuration[1] so the decoder will output copied YUV data instead of graphic buffers [2].

This also explains why bug 1159343 seems less serious after that patch.

[1] https://hg.mozilla.org/mozilla-central/rev/37c52f612c75#l5.49
[2] https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/gonk/GonkVideoDecoderManager.cpp?from=GonkVideoDecoderManager.cpp#232
TL;DR The texture of video frame is removed so there is no image to be shown. In [1], After calling UseTextures(), RemoveTexture() is called for each item in mBuffers. If one of them uses the same TextureClient as one item in |textures|, that texture will be removed by ImageHost::RemoveTextureHost().

As mentioned in comment 8, it only happens if graphic buffers are used. When that's true, TextureClient is bound with a graphic buffer in native window [2].

If configured with a native window, stagefright(MediaCodec/ACodec) will allocate and the dequeue some graphic buffers for output.

On B2G, every time when stagefright produces an output frame, MediaCodecProxy calls MediaCodec::getOutputGraphicBufferFromIndex() to get the buffer and give it to MediaFormatReader and MDSM for rendering. Once rendered, a recycle callback is called to notify that the buffer can be handed back to stagefright for producting new output later. A buffer will never be used for producing output and rendering at the same time.

Calling MediaCodec::flush() when seeking, however, causes a unexpected situation. flush() returns all known buffers to ACodec [3] without checking whether the buffer is used by client or not. When that happens, a buffer that's still rendering could be used by decoder for producing output at the same time. Once decoder completes producing, the buffer will be sent for rendering again and the texture bound with it will be used/removed as described above.

FWICT, MediaCodec::renderOutputBufferAndRelease(), which queues output buffer into native window for the consumer to acquire before using, is what stagefright expects its client to use for rendering output buffer. (ACodec never uses the buffer w/o dequeuing it from native window first [4])

The plan will be modifying GonkVideoDecoderManager and MediaCodecProxy to use renderOutputBufferAndRelease().

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/ImageClient.cpp?from=ImageClient.cpp#268
[2] https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/gonk/GonkVideoDecoderManager.cpp#234
[3] https://github.com/mozilla-b2g/platform_frameworks_av/blob/master/media/libstagefright/MediaCodec.cpp#L1480
[4] https://github.com/mozilla-b2g/platform_frameworks_av/blob/master/media/libstagefright/ACodec.cpp#L4322
Any update?
Flags: needinfo?(jolin)
(In reply to Ken Chang[:ken] from comment #10)
> Any update?

 I have a WIP patch that is still under testing. Will upload it once the final obstacle is overcame.
Flags: needinfo?(jolin)
If AsyncAskMediaCodec() run faster enough, mNativeWindow will be created after codecReserved() is called and we'll configure decoder w/o native window.
Comment on attachment 8676617 [details] [diff] [review]
Remove all references to unused task queue.

Media task queue is not needed anymore since all tasks will be dispatched to looper thread.
Attachment #8676617 - Flags: review?(jyavenard)
Comment on attachment 8676618 [details] [diff] [review]
Create native window early to avoid race condition.

Create native window before calling AsyncAskMediaCodec() ensures it's in correct state when accessed later in codecReserved().
Attachment #8676618 - Flags: review?(jyavenard)
Attachment #8676619 - Flags: review?(jyavenard)
Comment on attachment 8676620 [details] [diff] [review]
Use renderOutputBufferAndRelease() whenever possible to get graphic buffers.

This is the patch that actually fixes the blank image issue in this bug.
Attachment #8676620 - Flags: review?(jyavenard)
Attachment #8676620 - Flags: review?(bwu)
Attachment #8676617 - Flags: review?(jyavenard) → review+
Attachment #8676618 - Flags: review?(jyavenard) → review+
Comment on attachment 8676619 [details] [diff] [review]
Reset last decoded frame time on looper thread to avoid race condition.

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

It would make it much easier to figure out what the threading model is if:
1- There was some documentation in the header file on where a member is accessed from.
2- Have proper assert in all functions asserting that it is on the expected thread/taskqueue.

e.g. MOZ_ASSERT(OnLooperThread()); everywhere it needs to be.

Can you please open a new bug for doing this task?

thank you
Attachment #8676619 - Flags: review?(jyavenard) → review+
Comment on attachment 8676620 [details] [diff] [review]
Use renderOutputBufferAndRelease() whenever possible to get graphic buffers.

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

LGTM from a code style perspective, I don't know enough about Gonk to assess more than that.
Attachment #8676620 - Flags: review?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #19)
> Comment on attachment 8676619 [details] [diff] [review]
> Reset last decoded frame time on looper thread to avoid race condition.
> 
> Review of attachment 8676619 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It would make it much easier to figure out what the threading model is if:
> 1- There was some documentation in the header file on where a member is
> accessed from.
> 2- Have proper assert in all functions asserting that it is on the expected
> thread/taskqueue.
> 
> e.g. MOZ_ASSERT(OnLooperThread()); everywhere it needs to be.
> 
> Can you please open a new bug for doing this task?
> 
> thank you

Thanks a lot for the reviews. Bug 1216895 is filed for this.
Attachment #8676620 - Flags: review?(sotaro.ikeda.g)
Blocks: 1213566
Comment on attachment 8676620 [details] [diff] [review]
Use renderOutputBufferAndRelease() whenever possible to get graphic buffers.

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

I am trying to think if there is a simpler way to fix this problem. 
Will have some update tomorrow.
(In reply to John Lin [:jolin][:jhlin] from comment #9)
> 
> Calling MediaCodec::flush() when seeking, however, causes a unexpected
> situation. flush() returns all known buffers to ACodec [3] without checking
> whether the buffer is used by client or not. When that happens, a buffer
> that's still rendering could be used by decoder for producing output at the
> same time. Once decoder completes producing, the buffer will be sent for
> rendering again and the texture bound with it will be used/removed as
> described above.

Hmm, it is interesting that we did not recognize this problem. The reason why we did hack similar to OMXCodec is because of Bug 1009420 and MDSM requested to hold 3 video buffers to start playback. If the patch does not hit the problems, it seems better way of using MediaCodec. I am going to look into more.
Comment on attachment 8676620 [details] [diff] [review]
Use renderOutputBufferAndRelease() whenever possible to get graphic buffers.

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

::: dom/media/platforms/gonk/GonkVideoDecoderManager.cpp
@@ +355,5 @@
>  
> +  // Don't acquire too many buffers using renderOutputBufferAndRelease() because
> +  // decoder will block if there is no undequeued buffer in native window. Use
> +  // getOutputGraphicBufferFromIndex() instead.
> +  bool takeOutput = mRenderedBufferCount >= mMaxAcquiredBufferCount;

If we mixed 2 ways of graphic buffer acquisition, MediaCodec::flush()'s problem in Comment 9 seems not be addressed. Is there no problem about it?
Flags: needinfo?(jolin)
(In reply to Sotaro Ikeda [:sotaro] from comment #24)
> Comment on attachment 8676620 [details] [diff] [review]
> Use renderOutputBufferAndRelease() whenever possible to get graphic buffers.
> 
> Review of attachment 8676620 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/gonk/GonkVideoDecoderManager.cpp
> @@ +355,5 @@
> >  
> > +  // Don't acquire too many buffers using renderOutputBufferAndRelease() because
> > +  // decoder will block if there is no undequeued buffer in native window. Use
> > +  // getOutputGraphicBufferFromIndex() instead.
> > +  bool takeOutput = mRenderedBufferCount >= mMaxAcquiredBufferCount;
> 
> If we mixed 2 ways of graphic buffer acquisition, MediaCodec::flush()'s
> problem in Comment 9 seems not be addressed. Is there no problem about it?

 You're right. the fundamental problem, which flush() will return "taken" buffers, is not addressed here. The patch only makes the problem happen less frequently.
 It just occurs to me that we could just avoid the situation by making a copy of graphic buffer for the 1st frame after each flush. It will make seeking takes longer to complete but seems like a acceptable workaround.
Flags: needinfo?(jolin)
Isn't it better to call VideoFrameContainer::ClearFutureFrames() to return video buffers quickly, is it?
(In reply to John Lin [:jolin][:jhlin] from comment #25)
>  It just occurs to me that we could just avoid the situation by making a
> copy of graphic buffer for the 1st frame after each flush. It will make
> seeking takes longer to complete but seems like a acceptable workaround.

Can you explain more? Are you saying that "less frequently" is acceptable workaround?
Flags: needinfo?(jolin)
Flags: needinfo?(jolin)
Attachment #8676620 - Flags: review?(sotaro.ikeda.g)
Attachment #8676620 - Flags: review?(bwu)
(In reply to Sotaro Ikeda [:sotaro] from comment #27)
> (In reply to John Lin [:jolin][:jhlin] from comment #25)
> >  It just occurs to me that we could just avoid the situation by making a
> > copy of graphic buffer for the 1st frame after each flush. It will make
> > seeking takes longer to complete but seems like a acceptable workaround.
> 
> Can you explain more? Are you saying that "less frequently" is acceptable
> workaround?

 Sorry for not explaining my thought clear enough. I should have cancel the r? and obsolete the patch.

 No, I meant that my patch doesn't address the problem and blank image can still happen after applying it. And I have another idea to fix the problem, which is making a copy of 1st output buffer after every flush() call. Copying is slower than passing, but it at least makes sure the same buffer won't be sent for rendering after seeking. Another drawback of copying is that image tearing could be observed for a very short period of time before the copy is ready and sent, if the decoder updates content on previous buffer when compositor paints it.

 IMHO, to complete address the Gecko/stagefright buffer management problems (this one, bug 1009420, ...) that keep haunting us, it might be better to replace stagefright with our own wrapper of OpenMax components. Alfredo is working on this with adamdeath2010 (bug 1203859).
Ok, thanks for the explanation.
(In reply to Sotaro Ikeda [:sotaro] from comment #26)
> Isn't it better to call VideoFrameContainer::ClearFutureFrames() to return
> video buffers quickly, is it?

 Hmm... I'm not sure what you meant here. Did you suggest to call ClearFutureFrames() to release all buffers before seeking?
(In reply to John Lin [:jolin][:jhlin] from comment #30)
> (In reply to Sotaro Ikeda [:sotaro] from comment #26)
> > Isn't it better to call VideoFrameContainer::ClearFutureFrames() to return
> > video buffers quickly, is it?
> 
>  Hmm... I'm not sure what you meant here. Did you suggest to call
> ClearFutureFrames() to release all buffers before seeking?

Yes, it is ideal for seeking, I think. But it is not need to be done in this bug. 

If VideoFrameContainer owns multiple video buffers, it is better to release them except current one. It could add more buffers to hw decoders during seeking.
Attachment #8676620 - Attachment is obsolete: true
Comment on attachment 8679215 [details] [diff] [review]
WIP: make a copy of output buffer after flush().

WIP patch to copy the contents of decoder output buffer to another graphic buffer with issue that content process will crash from time to time. I'm checking what's wrong.
Attachment #8679215 - Flags: feedback?(sotaro.ikeda.g)
Attachment #8679215 - Flags: feedback?(bwu)
Call stack shows it crash at android::MediaBuffer::set_range() after the instance is released. The problem seems gone if I don't

 grallocClient->SetMediaBuffer(mVideoBuffer);

when CopyVideoBufferData().

Since we're not going to set its recycle callback, I suppose setMediaBuffer() is not necessary?
Flags: needinfo?(sotaro.ikeda.g)
Yes, in GonkVideoDecoderManager, if RecycleCallback() is not used for recycling setMediaBuffer() is not necessary. The release is done only by GonkVideoDecoderManager::ReleaseVideoBuffer(). But it seems better to change it like AutoReleaseAudioBuffer.
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #35)
> Yes, in GonkVideoDecoderManager, if RecycleCallback() is not used for
> recycling setMediaBuffer() is not necessary. The release is done only by
> GonkVideoDecoderManager::ReleaseVideoBuffer(). But it seems better to change
> it like AutoReleaseAudioBuffer.

 Neat! Will do that.
Comment on attachment 8679215 [details] [diff] [review]
WIP: make a copy of output buffer after flush().

Except GrallocTextureClient change, the patch seems good. TextureClient does not handle clearly in current gecko. The patch seemed to be affected from it.
I do not think it is a good idea to map HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS to SurfaceFormat::NV12. There could be another NV12 proprietary color format.
Attachment #8679215 - Flags: feedback?(sotaro.ikeda.g)
nical, how should we handle HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS in SurfaceFormat? In android, HAL_PIXEL_FORMAT_BLOB is used for similar purpose.
  http://androidxref.com/5.1.1_r6/xref/system/core/include/system/graphics.h#267
Flags: needinfo?(nical.bugzilla)
(In reply to Sotaro Ikeda [:sotaro] from comment #38)
> nical, how should we handle HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS in
> SurfaceFormat? In android, HAL_PIXEL_FORMAT_BLOB is used for similar purpose.

And HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED
(In reply to Sotaro Ikeda [:sotaro] from comment #38)
> nical, how should we handle HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS in
> SurfaceFormat? In android, HAL_PIXEL_FORMAT_BLOB is used for similar purpose.
>  
> http://androidxref.com/5.1.1_r6/xref/system/core/include/system/graphics.
> h#267

 I guess HAL_PIXEL_FORMAT_YCbCr_420_888 was added there also to support proprietary YUV formats.
(In reply to Sotaro Ikeda [:sotaro] from comment #38)
> nical, how should we handle HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS in
> SurfaceFormat? In android, HAL_PIXEL_FORMAT_BLOB is used for similar purpose.
>  
> http://androidxref.com/5.1.1_r6/xref/system/core/include/system/graphics.
> h#267

Forwarding the ni? to Bas

Perhaps something like gfx::SurfaceFormat::EXTERNAL or gfx::SurfaceFormat::YUV_EXTERNAL (a bit like gl) would do for the platform-specific formats that Moz2D won't draw into. I thought we had something like a YUVType enum to complement SurfaceFormat somewhere but I can't find it.
Flags: needinfo?(nical.bugzilla) → needinfo?(bas)
(In reply to Nicolas Silva [:nical] from comment #41)
> (In reply to Sotaro Ikeda [:sotaro] from comment #38)
> > nical, how should we handle HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS in
> > SurfaceFormat? In android, HAL_PIXEL_FORMAT_BLOB is used for similar purpose.
> >  
> > http://androidxref.com/5.1.1_r6/xref/system/core/include/system/graphics.
> > h#267
> 
> Forwarding the ni? to Bas
> 
> Perhaps something like gfx::SurfaceFormat::EXTERNAL or
> gfx::SurfaceFormat::YUV_EXTERNAL (a bit like gl) would do for the
> platform-specific formats that Moz2D won't draw into. I thought we had
> something like a YUVType enum to complement SurfaceFormat somewhere but I
> can't find it.

Something like that seems like the right idea, or SurfaceFormat::YUV_UNKNOWN or YUV_PROPRIETARY or something like that.
Flags: needinfo?(bas)
Comment on attachment 8679215 [details] [diff] [review]
WIP: make a copy of output buffer after flush().

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

Except graphics part, it looks good to me! I understand it is really hard to find a solution to make our render pipeline work with Android's MediaCodec/ACodec.   
IMHO, this is still a short-term workaround but better than the last one due to its simplicity and being able to fix the black screen effectively. One concern is how long that copy action takes and will it be acceptable for performance. 
We still need to find a good solution which doesn't need to copy data at first decoded frame. 
If this patch is taken as a fix, I will suggest creating another bug to find a better solution.
Attachment #8679215 - Flags: feedback?(bwu) → feedback+
John, when you apply these changes and have a new patch, make sure you ask Sotaro for a review first, he is much closer to your timezone and may give you a faster feedback.
If AsyncAskMediaCodec() run faster enough, mNativeWindow will be created after codecReserved() is called and we'll configure decoder w/o native window.
Attachment #8676617 - Attachment is obsolete: true
Attachment #8676618 - Attachment is obsolete: true
Attachment #8676619 - Attachment is obsolete: true
Attachment #8679215 - Attachment is obsolete: true
Comment on attachment 8679944 [details] [diff] [review]
Remove all references to unused task queue. r=jya

Carry r+ from jya.
Attachment #8679944 - Flags: review+
Comment on attachment 8679945 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

Carry r+ from jya.
Attachment #8679945 - Flags: review+
Comment on attachment 8679946 [details] [diff] [review]
Reset last decoded frame time on looper thread to avoid race condition. r=jya

Carry r+ from jya.
Attachment #8679946 - Flags: review+
Comment on attachment 8679947 [details] [diff] [review]
Refactor: use RAII to help manage output buffer lifecycle.

Address comment 35.
Attachment #8679947 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8679948 [details] [diff] [review]
Don't schedule decoder I/O task when there will be more input.

Also fix a bug that input EOS flag will not be set when there is already a task waiting.
Attachment #8679948 - Flags: review?(bwu)
Comment on attachment 8679949 [details] [diff] [review]
Make a copy of output buffer after flush().

Since we're very close to 2.5 release, changing a widely used class such as GrallocTextureClient doesn't sound like a good idea.
This patch implements copying graphic buffer locally for a quick fix. I'll file another bug to properly handle proprietary pixel formats in both GrallocImage and GrallocTexture*.
Attachment #8679949 - Flags: review?(sotaro.ikeda.g)
Attachment #8679949 - Flags: review?(bwu)
Comment on attachment 8679947 [details] [diff] [review]
Refactor: use RAII to help manage output buffer lifecycle.

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

::: dom/media/platforms/gonk/GonkVideoDecoderManager.cpp
@@ +334,5 @@
>      GVDM_LOG("Decoder is not inited");
>      return NS_ERROR_UNEXPECTED;
>    }
> +  MediaBuffer* outputBuffer;
> +  err = mDecoder->Output(&outputBuffer, READ_OUTPUT_BUFFER_TIMEOUT_US);

It seems better to add AutoReleaseMediaBuffer here like audio to make code more clear and robust. And call MediaBuffer::add_ref(), when we are going to register recycle callback to its TextureClient like OmxDecoder::ReadVideo()
https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/OmxDecoder.cpp#642

Current code implicitly expect MediaBuffer is create when return code is OK or ERROR_END_OF_STREAM.
Attachment #8679947 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8679949 [details] [diff] [review]
Make a copy of output buffer after flush().

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

::: dom/media/platforms/gonk/GonkVideoDecoderManager.cpp
@@ +143,5 @@
>    *v = nullptr;
>    RefPtr<VideoData> data;
>    int64_t timeUs;
>    int32_t keyFrame;
> +  bool needsCopy = mLastTime == kInvalidTime;

Can you add a comment why it is chosen and meed to mention about it is a temporary solution?
Just a note - if we are putting something in as a temporary solution, let's open a bug to fix it with a non-temporary solution.
Blocks: 1219210
(In reply to John Lin [:jolin][:jhlin] from comment #46)
> Created attachment 8679945 [details] [diff] [review]
> Create native window early to avoid race condition. r=jya
> 
> If AsyncAskMediaCodec() run faster enough, mNativeWindow will be created
> after codecReserved() is called and we'll configure decoder w/o native
> window.

This patch could not be applied cleanly.
If AsyncAskMediaCodec() run faster enough, mNativeWindow will be created after codecReserved() is called and we'll configure decoder w/o native window.
Comment on attachment 8679945 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

bitrotted because of bug 1219140.
Attachment #8679945 - Attachment is obsolete: true
Comment on attachment 8680427 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

Rebase and carry r+ from jya.
Attachment #8680427 - Flags: review+
Comment on attachment 8680427 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

Forgot to update commit message. :-$
Attachment #8680427 - Attachment is obsolete: true
If AsyncAllocateVideoMediaCodec() run faster enough, mNativeWindow will be created after codecReserved() is called and we'll configure decoder w/o native window.
Comment on attachment 8680428 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

(In reply to Sotaro Ikeda [:sotaro] from comment #60)
> (In reply to John Lin [:jolin][:jhlin] from comment #46)
> > Created attachment 8679945 [details] [diff] [review]
> > Create native window early to avoid race condition. r=jya
> > 
> > If AsyncAskMediaCodec() run faster enough, mNativeWindow will be created
> > after codecReserved() is called and we'll configure decoder w/o native
> > window.
> 
> This patch could not be applied cleanly.

 Sorry about that. Please try this one.
Attachment #8680428 - Flags: review+
Comment on attachment 8679948 [details] [diff] [review]
Don't schedule decoder I/O task when there will be more input.

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

Logically it looks good to me. 
Per discussion offline, just one suggestion to replace carrying input-eos in message with using a member variable. That should make codes simpler I think.
Attachment #8679948 - Flags: review?(bwu) → review+
After applying patches, I failed to play odd sized videos on aries and flame-kk. Before applying the patch, the videos are play backed normally.
 http://people.mozilla.org/~cpeterson/videos/big-buck-high.333x777.mp4
 http://people.mozilla.org/~cpeterson/videos/big-buck-high.777x333.mp4
(In reply to Sotaro Ikeda [:sotaro] from comment #68)
> After applying patches, I failed to play odd sized videos on aries and
> flame-kk. Before applying the patch, the videos are play backed normally.
>  http://people.mozilla.org/~cpeterson/videos/big-buck-high.333x777.mp4
>  http://people.mozilla.org/~cpeterson/videos/big-buck-high.777x333.mp4

 It's because Anroid YV12 doesn't allow odd sized buffer. Will update the allocation size arguments to address this. Thanks a lot!
(In reply to Sotaro Ikeda [:sotaro] from comment #57)
> Comment on attachment 8679947 [details] [diff] [review]
> Refactor: use RAII to help manage output buffer lifecycle.
> 
> Review of attachment 8679947 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/gonk/GonkVideoDecoderManager.cpp
> @@ +334,5 @@
> >      GVDM_LOG("Decoder is not inited");
> >      return NS_ERROR_UNEXPECTED;
> >    }
> > +  MediaBuffer* outputBuffer;
> > +  err = mDecoder->Output(&outputBuffer, READ_OUTPUT_BUFFER_TIMEOUT_US);
> 
> It seems better to add AutoReleaseMediaBuffer here like audio to make code
> more clear and robust. And call MediaBuffer::add_ref(), when we are going to
> register recycle callback to its TextureClient like OmxDecoder::ReadVideo()
> https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/OmxDecoder.
> cpp#642
> 

 Is add_ref()/release() needed here? IIUC,
 - MediaBuffer::mRefCount is 0 [1] when constructed in [2] and MediaBuffer::release() only is called in [3] during RecycleCallback()
 - Unlike OMXCodec, MediaCodecProxy doesn't call setObserver(), so release() will branch to [4] and delete the instance directly.

 It seems to me that add_ref()/release doesn't affect the lifecycle of MediaBuffer if sestObserver() doesn't not get involved. Am I missing something here?

[1] http://androidxref.com/6.0.0_r1/xref/frameworks/av/media/libstagefright/MediaBuffer.cpp#65
[2] https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/MediaCodecProxy.cpp?from=MediaCodecProxy.cpp#643
[3] https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/MediaCodecProxy.cpp?from=MediaCodecProxy.cpp#668
[4] http://androidxref.com/6.0.0_r1/xref/frameworks/av/media/libstagefright/MediaBuffer.cpp#89
Flags: needinfo?(sotaro.ikeda.g)
John, 
Do you have a number about how much time for copying the first frame with your patch? 
Just want to have an idea how much performance will be impacted.
(In reply to John Lin [:jolin][:jhlin] from comment #70)
> (In reply to Sotaro Ikeda [:sotaro] from comment #57)
> > Comment on attachment 8679947 [details] [diff] [review]
> > Refactor: use RAII to help manage output buffer lifecycle.
> > 
> > Review of attachment 8679947 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/media/platforms/gonk/GonkVideoDecoderManager.cpp
> > @@ +334,5 @@
> > >      GVDM_LOG("Decoder is not inited");
> > >      return NS_ERROR_UNEXPECTED;
> > >    }
> > > +  MediaBuffer* outputBuffer;
> > > +  err = mDecoder->Output(&outputBuffer, READ_OUTPUT_BUFFER_TIMEOUT_US);
> > 
> > It seems better to add AutoReleaseMediaBuffer here like audio to make code
> > more clear and robust. And call MediaBuffer::add_ref(), when we are going to
> > register recycle callback to its TextureClient like OmxDecoder::ReadVideo()
> > https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/OmxDecoder.
> > cpp#642
> > 
> 
>  Is add_ref()/release() needed here? IIUC,
>  - MediaBuffer::mRefCount is 0 [1] when constructed in [2] and
> MediaBuffer::release() only is called in [3] during RecycleCallback()
>  - Unlike OMXCodec, MediaCodecProxy doesn't call setObserver(), so release()
> will branch to [4] and delete the instance directly.
> 
>  It seems to me that add_ref()/release doesn't affect the lifecycle of
> MediaBuffer if sestObserver() doesn't not get involved. Am I missing
> something here?
> 
> [1]
> http://androidxref.com/6.0.0_r1/xref/frameworks/av/media/libstagefright/
> MediaBuffer.cpp#65
> [2]
> https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/MediaCodecProxy.
> cpp?from=MediaCodecProxy.cpp#643
> [3]
> https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/MediaCodecProxy.
> cpp?from=MediaCodecProxy.cpp#668
> [4]
> http://androidxref.com/6.0.0_r1/xref/frameworks/av/media/libstagefright/
> MediaBuffer.cpp#89

I just wanted to change the place of AutoReleaseMediaBuffer of video like audio one. If it requests correct refcounting, it is necessary to do.
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Blake Wu [:bwu][:blakewu] from comment #71)
> John, 
> Do you have a number about how much time for copying the first frame with
> your patch? 
> Just want to have an idea how much performance will be impacted.

It could be nice to check how many copy happens for each video thumbnail generation.
> 
> I just wanted to change the place of AutoReleaseMediaBuffer of video like
> audio one. If it requests correct refcounting, it is necessary to do.

I changed my mind, it is not necessary to be done in this bug if you are not working for it already. Because, it is not related to this bug's problem.
Attachment #8679947 - Flags: review+
Attachment #8679949 - Flags: review?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #74)
> > 
> > I just wanted to change the place of AutoReleaseMediaBuffer of video like
> > audio one. If it requests correct refcounting, it is necessary to do.
> 
> I changed my mind, it is not necessary to be done in this bug if you are not
> working for it already. Because, it is not related to this bug's problem.

 I think it's better to use AutoReleaseMediaBuffer for video too. The question about add_ref()/release() I brought up is because after adding them w/o setObserver(), the check at [1] failed and it seems to me add_ref()/release() is designed to work with setObserver().

[1] http://androidxref.com/6.0.0_r1/xref/frameworks/av/media/libstagefright/MediaBuffer.cpp#89
(In reply to John Lin [:jolin][:jhlin] from comment #69)
> (In reply to Sotaro Ikeda [:sotaro] from comment #68)
> > After applying patches, I failed to play odd sized videos on aries and
> > flame-kk. Before applying the patch, the videos are play backed normally.
> >  http://people.mozilla.org/~cpeterson/videos/big-buck-high.333x777.mp4
> >  http://people.mozilla.org/~cpeterson/videos/big-buck-high.777x333.mp4
> 
>  It's because Anroid YV12 doesn't allow odd sized buffer. Will update the
> allocation size arguments to address this. Thanks a lot!

 Supporting odd sized buffer is more complicated than I thought. After making YV12 buffer even sized the UV planes doesn't aligned with Y. Then I tried copying to RGB565 graphic buffer using android::ColorConverter but still cannot get correct results.
(In reply to Blake Wu [:bwu][:blakewu] from comment #71)
> John, 
> Do you have a number about how much time for copying the first frame with
> your patch? 
> Just want to have an idea how much performance will be impacted.

 Most of the time copying takes less than 100ms.
When video size is odd, gralloc buffer size and valid video size is always different.
Turns out I didn't modify width/height values in aPicture to match the actual buffer size. :-$
If AsyncAllocateVideoMediaCodec() run faster enough, mNativeWindow will be created after codecReserved() is called and we'll configure decoder w/o native window.
Attachment #8679944 - Attachment is obsolete: true
Attachment #8679946 - Attachment is obsolete: true
Attachment #8679947 - Attachment is obsolete: true
Attachment #8679948 - Attachment is obsolete: true
Attachment #8679949 - Attachment is obsolete: true
Attachment #8679949 - Flags: review?(bwu)
Attachment #8680428 - Attachment is obsolete: true
Comment on attachment 8681051 [details] [diff] [review]
Make a copy of output buffer after flush().

Address review comments and fix odd sized video problem.
Attachment #8681051 - Flags: review?(sotaro.ikeda.g)
Attachment #8681051 - Flags: review?(bwu)
Comment on attachment 8681051 [details] [diff] [review]
Make a copy of output buffer after flush().

Wrong patch.
Attachment #8681051 - Flags: review?(sotaro.ikeda.g)
Attachment #8681051 - Flags: review?(bwu)
Attachment #8681051 - Attachment is obsolete: true
Comment on attachment 8681046 [details] [diff] [review]
Remove all references to unused task queue. r=jya

Final patch to land.
Attachment #8681046 - Flags: review+
Comment on attachment 8681047 [details] [diff] [review]
Reset last decoded frame time on looper thread to avoid race condition. r=jya

Final patch to land.
Attachment #8681047 - Flags: review+
Comment on attachment 8681048 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

Final patch to land.
Attachment #8681048 - Flags: review+
Comment on attachment 8681049 [details] [diff] [review]
Don't schedule decoder I/O task when there will be more input. r=bwu

Rebase and carry r+ from bwu.
Attachment #8681049 - Flags: review+
Comment on attachment 8681050 [details] [diff] [review]
Refactor: use RAII to help manage output buffer lifecycle. r=sotaro

Rebase and carry r+ from sotaro.
Attachment #8681050 - Flags: review+
Comment on attachment 8681057 [details] [diff] [review]
Make a copy of output buffer after flush().

Address review comments and handle odd sized video.
Attachment #8681057 - Flags: review?(sotaro.ikeda.g)
Attachment #8681057 - Flags: review?(bwu)
Comment on attachment 8681057 [details] [diff] [review]
Make a copy of output buffer after flush().

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

::: gfx/layers/opengl/GrallocTextureClient.cpp
@@ +294,5 @@
>      format = HAL_PIXEL_FORMAT_YV12;
>      break;
> +  case gfx::SurfaceFormat::NV12:
> +    format = GrallocImage::HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS;
> +    break;

Is there a reason why is the change revive? It did not exist in a previous patch.
(In reply to Sotaro Ikeda [:sotaro] from comment #95)
> Comment on attachment 8681057 [details] [diff] [review]
> Make a copy of output buffer after flush().
> 
> Review of attachment 8681057 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/opengl/GrallocTextureClient.cpp
> @@ +294,5 @@
> >      format = HAL_PIXEL_FORMAT_YV12;
> >      break;
> > +  case gfx::SurfaceFormat::NV12:
> > +    format = GrallocImage::HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS;
> > +    break;
> 
> Is there a reason why is the change revive? It did not exist in a previous
> patch.

 Oops. I made a mistake of forgetting to discard unwanted local changes.
 It's not supposed to be in the patch.
 Sorry about that. Please ignore it and I'll fix in future patch.
Comment on attachment 8681057 [details] [diff] [review]
Make a copy of output buffer after flush().

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

Sorry. I am not following the discussion about the changes in this patch and cannot help review this patch. Sotaro is the best guy to review this patch.
Attachment #8681057 - Flags: review?(bwu)
Attachment #8681057 - Flags: review?(sotaro.ikeda.g)
Attachment #8681057 - Attachment is obsolete: true
Comment on attachment 8681235 [details] [diff] [review]
Make a copy of output buffer after flush().

Remove unwanted changes from the patch.
Attachment #8681235 - Flags: review?(sotaro.ikeda.g)
(In reply to John Lin [:jolin][:jhlin] from comment #9)
> TL;DR The texture of video frame is removed so there is no image to be
> shown. In [1], After calling UseTextures(), RemoveTexture() is called for
> each item in mBuffers. If one of them uses the same TextureClient as one
> item in |textures|, that texture will be removed by
> ImageHost::RemoveTextureHost().
Sotaro,
Would it be ok or simpler to modify gfx codes to not remove the texture if it is the same to the previous one?
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Blake Wu [:bwu][:blakewu] from comment #100)
> (In reply to John Lin [:jolin][:jhlin] from comment #9)
> > TL;DR The texture of video frame is removed so there is no image to be
> > shown. In [1], After calling UseTextures(), RemoveTexture() is called for
> > each item in mBuffers. If one of them uses the same TextureClient as one
> > item in |textures|, that texture will be removed by
> > ImageHost::RemoveTextureHost().
> Sotaro,
> Would it be ok or simpler to modify gfx codes to not remove the texture if
> it is the same to the previous one?
Just wonder whether it is a simpler way to fix this bug or not.
(In reply to Blake Wu [:bwu][:blakewu] from comment #100)
> (In reply to John Lin [:jolin][:jhlin] from comment #9)
> > TL;DR The texture of video frame is removed so there is no image to be
> > shown. In [1], After calling UseTextures(), RemoveTexture() is called for
> > each item in mBuffers. If one of them uses the same TextureClient as one
> > item in |textures|, that texture will be removed by
> > ImageHost::RemoveTextureHost().
> Sotaro,
> Would it be ok or simpler to modify gfx codes to not remove the texture if
> it is the same to the previous one?

I do not think this happen when we apply the path. I could happen only when flush() make buffer ownership wrong. If the ownership works correctly, the following code in ImageClientSingle::UpdateImage() should prevent it happen in Image level.

>  for (auto& img : images) {
>    Image* image = img.mImage;
>    RefPtr<TextureClient> texture = image->GetTextureClient(this);
>
>    for (int32_t i = mBuffers.Length() - 1; i >= 0; --i) {
>      if (mBuffers[i].mImageSerial == image->GetSerial()) {
>        if (texture) {
>          MOZ_ASSERT(texture == mBuffers[i].mTextureClient);
>        } else {
>          texture = mBuffers[i].mTextureClient;
>        }
>        // Remove this element from mBuffers so mBuffers only contains
>        // images that aren't present in 'images'
>        mBuffers.RemoveElementAt(i);
>      }
>    }

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/ImageClient.cpp#157
Flags: needinfo?(sotaro.ikeda.g)
Comment on attachment 8681235 [details] [diff] [review]
Make a copy of output buffer after flush().

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

::: dom/media/platforms/gonk/GonkVideoDecoderManager.cpp
@@ +148,5 @@
> +  // Bug 1199809: workaround to avoid sending the graphic buffer by making a
> +  // copy of output buffer after calling flush(). Bug 1203859 was created to
> +  // reimplementing Gonk PDM on top of OpenMax IL directly. Its buffer
> +  // management will work better with Gecko and solve problems like this.
> +  bool needsCopy = mLastTime == kInvalidTime;

Can't we avoid copying for the first video, can we? It could decrease performance impact.
Attachment #8681235 - Flags: review?(sotaro.ikeda.g)
Attachment #8681235 - Attachment is obsolete: true
Comment on attachment 8681785 [details] [diff] [review]
Make a copy of output buffer after flush().

Introduce dedicated flag variable to indicate the need of copying buffer. This also avoids copying 1st decoded frame.
Unfortunately it doesn't help the video app thumbnail generation case because it always seek.
Attachment #8681785 - Flags: review?(sotaro.ikeda.g)
(In reply to John Lin [:jolin][:jhlin] from comment #105)
> Comment on attachment 8681785 [details] [diff] [review]
> Make a copy of output buffer after flush().
> 
> Introduce dedicated flag variable to indicate the need of copying buffer.
> This also avoids copying 1st decoded frame.
> Unfortunately it doesn't help the video app thumbnail generation case
> because it always seek.

I know, but we could reduce one copy.
Comment on attachment 8681785 [details] [diff] [review]
Make a copy of output buffer after flush().

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

Looks good1
Attachment #8681785 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8681785 [details] [diff] [review]
Make a copy of output buffer after flush().

Replaced by patch to land.
Attachment #8681785 - Attachment is obsolete: true
Comment on attachment 8681800 [details] [diff] [review]
Make a copy of output buffer after flush(). r=sotaro

Carry r+ from sotaro.
Attachment #8681800 - Flags: review+
Keywords: verifyme
I've verified this issue is fixed on the latest Aries and Flame 2.6 builds.  It is not yet on the 2.5 branch so I cannot verify that.  John is this going to be uplifted there?

Setting overall status to verified but leaving verifyme tag til 2.5 is verified or confirmed to not get this fix per our normal procedure.

Actual Results:  The preview was always shown when seeking or using the FF/REW buttons.

Environmental Variables:
Device: Flame 2.6
BuildID: 20151103030244
Gaia: 06de78d2c61c084956640c480280ba518b2fe29f
Gecko: bb4d614a0b09bcb9738c151dccfcd9b3857a6a7c
Gonk: 205ac4204bbbb2098a8046444acba551ba5dc75a
Version: 45.0a1 (2.6) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:45.0) Gecko/45.0 Firefox/45.0

Environmental Variables:
Device: Aries 2.6
BuildID: 20151103140850
Gaia: 06de78d2c61c084956640c480280ba518b2fe29f
Gecko: 59a6ad6a921f4809dfc37d943d765300c65721e5
Gonk: a19052e4389c3ae2d8fc3e7a74a475401baacc56
Version: 45.0a1 (2.6) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:45.0) Gecko/45.0 Firefox/45.0
Status: RESOLVED → VERIFIED
Flags: needinfo?(jolin)
Flags: needinfo?(jmercado)
Comment on attachment 8681046 [details] [diff] [review]
Remove all references to unused task queue. r=jya

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1199809
User impact if declined: blank image after seeking
Testing completed: the preview was always shown when seeking or using the FF/REW buttons.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #8681046 - Flags: approval‑mozilla‑b2g44?
Comment on attachment 8681047 [details] [diff] [review]
Reset last decoded frame time on looper thread to avoid race condition. r=jya

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

Bug caused by (feature/regressing bug #): bug 1199809
User impact if declined: blank image after seeking
Testing completed: the preview was always shown when seeking or using the FF/REW buttons.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #8681047 - Flags: approval‑mozilla‑b2g44?
Comment on attachment 8681048 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

Bug caused by (feature/regressing bug #): bug 1199809
User impact if declined: blank image after seeking
Testing completed: the preview was always shown when seeking or using the FF/REW buttons.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #8681048 - Flags: approval‑mozilla‑b2g44?
Comment on attachment 8681049 [details] [diff] [review]
Don't schedule decoder I/O task when there will be more input. r=bwu

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

Bug caused by (feature/regressing bug #): bug 1199809
User impact if declined: blank image after seeking
Testing completed: the preview was always shown when seeking or using the FF/REW buttons.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #8681049 - Flags: approval‑mozilla‑b2g44?
Comment on attachment 8681050 [details] [diff] [review]
Refactor: use RAII to help manage output buffer lifecycle. r=sotaro

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

Bug caused by (feature/regressing bug #): bug 1199809
User impact if declined: blank image after seeking
Testing completed: the preview was always shown when seeking or using the FF/REW buttons.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #8681050 - Flags: approval‑mozilla‑b2g44?
Comment on attachment 8681800 [details] [diff] [review]
Make a copy of output buffer after flush(). r=sotaro

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

Bug caused by (feature/regressing bug #): bug 1199809
User impact if declined: blank image after seeking
Testing completed: the preview was always shown when seeking or using the FF/REW buttons.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Flags: needinfo?(jolin)
Attachment #8681800 - Flags: approval‑mozilla‑b2g44?
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
This issue is verified fixed in the latest 2.5 Aries and Flame builds.

Actual Results: Manually scrubbing the bar or using the FF/REW buttons properly shows preview images and not black screens.

Environmental Variables:
Device: Aries 2.5
BuildID: 20151109214310
Gaia: 9f823b3b2902558bad224384e1abddd1bb35d93a
Gecko: 5d8c923e44f54f1da2b1b87bf1400a4a43d1ef1a
Gonk: a19052e4389c3ae2d8fc3e7a74a475401baacc56
Version: 44.0a2 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

Environmental Variables:
Device: Flame 2.5
BuildID: 20151109004552
Gaia: cf646c52bb947af28329b0a100df91d1b1f2a907
Gecko: 4eafef5b80f8985c94c4a067f130d37513e1a581
Gonk: 205ac4204bbbb2098a8046444acba551ba5dc75a
Version: 44.0a2 (2.5) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0
Flags: needinfo?(jmercado) → needinfo?(ktucker)
Flags: needinfo?(ktucker)
Comment on attachment 8681046 [details] [diff] [review]
Remove all references to unused task queue. r=jya

Approved for 2.5

Thanks
Attachment #8681046 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Comment on attachment 8681047 [details] [diff] [review]
Reset last decoded frame time on looper thread to avoid race condition. r=jya

Approved for 2.5

Thanks
Attachment #8681047 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Comment on attachment 8681048 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

Approved for 2.5

Thanks
Attachment #8681048 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Comment on attachment 8681049 [details] [diff] [review]
Don't schedule decoder I/O task when there will be more input. r=bwu

Approved for 2.5

Thanks
Attachment #8681049 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Comment on attachment 8681050 [details] [diff] [review]
Refactor: use RAII to help manage output buffer lifecycle. r=sotaro

Approved for 2.5

Thanks
Attachment #8681050 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Comment on attachment 8681800 [details] [diff] [review]
Make a copy of output buffer after flush(). r=sotaro

Approved for 2.5

Thanks
Attachment #8681800 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Depends on: 1227415
You need to log in before you can comment on or make changes to this bug.