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

Status: VERIFIED FIXED

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

Description

[No-Jun Park [:njpark]]

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

Comment 1

[No-Jun Park [:njpark]]

[Blocking Requested - why for this release]:
functional regression

Comment 2

[No-Jun Park [:njpark]]

Video link:

http://youtu.be/m7rU_nuh7xw

Comment 3

[Martin Shuman [:Marty]]

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

Comment 4

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Alfredo this issue seems to have been caused by the changes for either bug 1179667 or bug 1195625.  Can you please take a look?

Comment 5

[John Lin [:jhlin][:jolin]]

I'll check it. Thanks for identifying the commits that cause the issue.

Comment 6

[Mahendra Potharaju [:mahe]]

Regression, Blocking for 2.5 with P2 priority.

Comment 8

[John Lin [:jhlin][:jolin]]

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

Comment 9

[John Lin [:jhlin][:jolin]]

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

Comment 10

[Ken Chang[:kenkai]]

Any update?

Comment 11

[John Lin [:jhlin][: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.

Comment 12

[John Lin [:jhlin][:jolin]]

Attachment: Remove all references to unused task queue.

Comment 13

[John Lin [:jhlin][:jolin]]

Attachment: Create native window early to avoid race condition.

If AsyncAskMediaCodec() run faster enough, mNativeWindow will be created after codecReserved() is called and we'll configure decoder w/o native window.

Comment 14

[John Lin [:jhlin][:jolin]]

Attachment: Reset last decoded frame time on looper thread to avoid race condition.

Comment 15

[John Lin [:jhlin][:jolin]]

Attachment: Use renderOutputBufferAndRelease() whenever possible to get graphic buffers.

Comment 16

[John Lin [:jhlin][:jolin]]

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.

Comment 17

[John Lin [:jhlin][:jolin]]

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().

Comment 18

[John Lin [:jhlin][:jolin]]

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.

Comment 19

[Jean-Yves Avenard [:jya]]

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

Comment 20

[Jean-Yves Avenard [:jya]]

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.

Comment 21

[John Lin [:jhlin][:jolin]]

(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.

Comment 22

[Blake Wu [:bwu][:blakewu]]

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.

Comment 23

[Sotaro Ikeda [:sotaro]]

(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 24

[Sotaro Ikeda [:sotaro]]

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?

Comment 25

[John Lin [:jhlin][: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.

Comment 26

[Sotaro Ikeda [:sotaro]]

Isn't it better to call VideoFrameContainer::ClearFutureFrames() to return video buffers quickly, is it?

Comment 27

[Sotaro Ikeda [:sotaro]]

(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?

Comment 28

[John Lin [:jhlin][:jolin]]

(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).

Comment 29

[Sotaro Ikeda [:sotaro]]

Ok, thanks for the explanation.

Comment 30

[John Lin [:jhlin][:jolin]]

(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?

Comment 31

[Sotaro Ikeda [:sotaro]]

(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.

Comment 32

[John Lin [:jhlin][:jolin]]

Attachment: WIP: make a copy of output buffer after flush().

Comment 33

[John Lin [:jhlin][:jolin]]

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.

Comment 34

[John Lin [:jhlin][:jolin]]

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?

Comment 35

[Sotaro Ikeda [:sotaro]]

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.

Comment 36

[John Lin [:jhlin][:jolin]]

(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 37

[Sotaro Ikeda [:sotaro]]

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.

Comment 38

[Sotaro Ikeda [:sotaro]]

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

Comment 39

[Sotaro Ikeda [:sotaro]]

(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

Comment 40

[John Lin [:jhlin][:jolin]]

(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.

Comment 41

[Nicolas Silva [:nical]]

(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.

Comment 42

[Bas Schouten (:bas.schouten)]

(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.

Comment 43

[Blake Wu [:bwu][:blakewu]]

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.

Comment 44

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 45

[John Lin [:jhlin][:jolin]]

Attachment: Remove all references to unused task queue. r=jya

Comment 46

[John Lin [:jhlin][:jolin]]

Attachment: 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.

Comment 47

[John Lin [:jhlin][:jolin]]

Attachment: Reset last decoded frame time on looper thread to avoid race condition. r=jya

Comment 48

[John Lin [:jhlin][:jolin]]

Attachment: Refactor: use RAII to help manage output buffer lifecycle.

Comment 49

[John Lin [:jhlin][:jolin]]

Attachment: Don't schedule decoder I/O task when there will be more input.

Comment 50

[John Lin [:jhlin][:jolin]]

Attachment: Make a copy of output buffer after flush().

Comment 51

[John Lin [:jhlin][:jolin]]

Comment on attachment 8679944 [details] [diff] [review]
Remove all references to unused task queue. r=jya

Carry r+ from jya.

Comment 52

[John Lin [:jhlin][:jolin]]

Comment on attachment 8679945 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

Carry r+ from jya.

Comment 53

[John Lin [:jhlin][:jolin]]

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.

Comment 54

[John Lin [:jhlin][:jolin]]

Comment on attachment 8679947 [details] [diff] [review]
Refactor: use RAII to help manage output buffer lifecycle.

Address comment 35.

Comment 55

[John Lin [:jhlin][:jolin]]

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.

Comment 56

[John Lin [:jhlin][:jolin]]

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*.

Comment 57

[Sotaro Ikeda [:sotaro]]

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.

Comment 58

[Sotaro Ikeda [:sotaro]]

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?

Comment 59

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 60

[Sotaro Ikeda [:sotaro]]

(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.

Comment 61

[John Lin [:jhlin][:jolin]]

Attachment: 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.

Comment 62

[John Lin [:jhlin][:jolin]]

Comment on attachment 8679945 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

bitrotted because of bug 1219140.

Comment 63

[John Lin [:jhlin][:jolin]]

Comment on attachment 8680427 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

Rebase and carry r+ from jya.

Comment 64

[John Lin [:jhlin][:jolin]]

Comment on attachment 8680427 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

Forgot to update commit message. :-$

Comment 65

[John Lin [:jhlin][:jolin]]

Attachment: Create native window early to avoid race condition. r=jya

If AsyncAllocateVideoMediaCodec() run faster enough, mNativeWindow will be created after codecReserved() is called and we'll configure decoder w/o native window.

Comment 66

[John Lin [:jhlin][:jolin]]

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.

Comment 67

[Blake Wu [:bwu][:blakewu]]

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.

Comment 68

[Sotaro Ikeda [:sotaro]]

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

Comment 69

[John Lin [:jhlin][:jolin]]

(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!

Comment 70

[John Lin [:jhlin][:jolin]]

(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

Comment 71

[Blake Wu [:bwu][:blakewu]]

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.

Comment 72

[Sotaro Ikeda [:sotaro]]

(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.

Comment 73

[Sotaro Ikeda [:sotaro]]

(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.

Comment 74

[Sotaro Ikeda [:sotaro]]

> 
> 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.

Comment 75

[John Lin [:jhlin][:jolin]]

(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

Comment 76

[John Lin [:jhlin][:jolin]]

(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.

Comment 77

[John Lin [:jhlin][:jolin]]

(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.

Comment 78

[Sotaro Ikeda [:sotaro]]

When video size is odd, gralloc buffer size and valid video size is always different.

Comment 79

[John Lin [:jhlin][:jolin]]

Turns out I didn't modify width/height values in aPicture to match the actual buffer size. :-$

Comment 80

[John Lin [:jhlin][:jolin]]

Attachment: Remove all references to unused task queue. r=jya

Comment 81

[John Lin [:jhlin][:jolin]]

Attachment: Reset last decoded frame time on looper thread to avoid race condition. r=jya

Comment 82

[John Lin [:jhlin][:jolin]]

Attachment: Create native window early to avoid race condition. r=jya

If AsyncAllocateVideoMediaCodec() run faster enough, mNativeWindow will be created after codecReserved() is called and we'll configure decoder w/o native window.

Comment 83

[John Lin [:jhlin][:jolin]]

Attachment: Don't schedule decoder I/O task when there will be more input. r=bwu

Comment 84

[John Lin [:jhlin][:jolin]]

Attachment: Refactor: use RAII to help manage output buffer lifecycle. r=sotaro

Comment 85

[John Lin [:jhlin][:jolin]]

Attachment: Make a copy of output buffer after flush().

Comment 86

[John Lin [:jhlin][:jolin]]

Comment on attachment 8681051 [details] [diff] [review]
Make a copy of output buffer after flush().

Address review comments and fix odd sized video problem.

Comment 87

[John Lin [:jhlin][:jolin]]

Comment on attachment 8681051 [details] [diff] [review]
Make a copy of output buffer after flush().

Wrong patch.

Comment 88

[John Lin [:jhlin][:jolin]]

Attachment: Make a copy of output buffer after flush().

Comment 89

[John Lin [:jhlin][:jolin]]

Comment on attachment 8681046 [details] [diff] [review]
Remove all references to unused task queue. r=jya

Final patch to land.

Comment 90

[John Lin [:jhlin][:jolin]]

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.

Comment 91

[John Lin [:jhlin][:jolin]]

Comment on attachment 8681048 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

Final patch to land.

Comment 92

[John Lin [:jhlin][:jolin]]

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.

Comment 93

[John Lin [:jhlin][:jolin]]

Comment on attachment 8681050 [details] [diff] [review]
Refactor: use RAII to help manage output buffer lifecycle. r=sotaro

Rebase and carry r+ from sotaro.

Comment 94

[John Lin [:jhlin][:jolin]]

Comment on attachment 8681057 [details] [diff] [review]
Make a copy of output buffer after flush().

Address review comments and handle odd sized video.

Comment 95

[Sotaro Ikeda [:sotaro]]

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.

Comment 96

[John Lin [:jhlin][:jolin]]

(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 97

[Blake Wu [:bwu][:blakewu]]

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.

Comment 98

[John Lin [:jhlin][:jolin]]

Attachment: Make a copy of output buffer after flush().

Comment 99

[John Lin [:jhlin][:jolin]]

Comment on attachment 8681235 [details] [diff] [review]
Make a copy of output buffer after flush().

Remove unwanted changes from the patch.

Comment 100

[Blake Wu [:bwu][:blakewu]]

(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?

Comment 101

[Blake Wu [:bwu][:blakewu]]

(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.

Comment 102

[Sotaro Ikeda [:sotaro]]

(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

Comment 103

[Sotaro Ikeda [:sotaro]]

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.

Comment 104

[John Lin [:jhlin][:jolin]]

Attachment: Make a copy of output buffer after flush().

Comment 105

[John Lin [:jhlin][:jolin]]

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.

Comment 106

[Sotaro Ikeda [:sotaro]]

(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 107

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8681785 [details] [diff] [review]
Make a copy of output buffer after flush().

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

Looks good1

Comment 108

[John Lin [:jhlin][:jolin]]

Attachment: Make a copy of output buffer after flush(). r=sotaro

Comment 109

[John Lin [:jhlin][:jolin]]

Comment on attachment 8681785 [details] [diff] [review]
Make a copy of output buffer after flush().

Replaced by patch to land.

Comment 110

[John Lin [:jhlin][:jolin]]

Comment on attachment 8681800 [details] [diff] [review]
Make a copy of output buffer after flush(). r=sotaro

Carry r+ from sotaro.

Comment 111

[John Lin [:jhlin][:jolin]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8a607e7d220

Comment 112

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20862dad6450
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ee0c5748fe3
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc8656a7ae65
https://hg.mozilla.org/integration/mozilla-inbound/rev/294042940ba9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc683f25a53
https://hg.mozilla.org/integration/mozilla-inbound/rev/628b84e68ac7

Comment 113

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/20862dad6450
https://hg.mozilla.org/mozilla-central/rev/9ee0c5748fe3
https://hg.mozilla.org/mozilla-central/rev/cc8656a7ae65
https://hg.mozilla.org/mozilla-central/rev/294042940ba9
https://hg.mozilla.org/mozilla-central/rev/9fc683f25a53
https://hg.mozilla.org/mozilla-central/rev/628b84e68ac7

Comment 114

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

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

Comment 115

[John Lin [:jhlin][:jolin]]

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

Comment 116

[John Lin [:jhlin][:jolin]]

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

Comment 117

[John Lin [:jhlin][:jolin]]

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

Comment 118

[John Lin [:jhlin][:jolin]]

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

Comment 119

[John Lin [:jhlin][:jolin]]

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

Comment 120

[John Lin [:jhlin][:jolin]]

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

Comment 121

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/20862dad6450
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9ee0c5748fe3
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/cc8656a7ae65
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/294042940ba9
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9fc683f25a53
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/628b84e68ac7

Comment 122

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 123

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/569e89891d9b
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/95facb0cc5fe
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/bc611cd77980
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/15dd82afc812
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9dbdf477c66c
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/f6faf0c7c6a2

Comment 124

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

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

Comment 125

[Mahendra Potharaju [:mahe]]

Comment on attachment 8681046 [details] [diff] [review]
Remove all references to unused task queue. r=jya

Approved for 2.5

Thanks

Comment 126

[Mahendra Potharaju [:mahe]]

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

Comment 127

[Mahendra Potharaju [:mahe]]

Comment on attachment 8681048 [details] [diff] [review]
Create native window early to avoid race condition. r=jya

Approved for 2.5

Thanks

Comment 128

[Mahendra Potharaju [:mahe]]

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

Comment 129

[Mahendra Potharaju [:mahe]]

Comment on attachment 8681050 [details] [diff] [review]
Refactor: use RAII to help manage output buffer lifecycle. r=sotaro

Approved for 2.5

Thanks

Comment 130

[Mahendra Potharaju [:mahe]]

Comment on attachment 8681800 [details] [diff] [review]
Make a copy of output buffer after flush(). r=sotaro

Approved for 2.5

Thanks