User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/36.0.1985.125 Safari/537.36 Steps to reproduce: 1. Copy attached VP9 video file to SDcard. 2. Insert SDcard to device and execute video player. 3. Player will scan video file. 4. If player success scanning, remove SDcard and insert SDcard again to scan file. 5. If you repeat step4, sometime VP9 video file is not listed up. Actual results: [Code level step of issue case] 1. Player try to scan VP9 video file. 2. During scaning, gecko Calls MediaDecoderStateMachine::DecodeMetadata 3. In DecodeMetadata function, call MediaOMXReader::ReadMetadata. 4. ReadMetadata calls "OmxDecoder::TryLoad()". 5. But when issue is occurred, videoSouce(OMXCodecProxy.cpp) is waiting resources. (At this time, state is "CLIENT_STATE_WAIT_FOR_RESOURCE") 6. So, to wait resources, OmxDecoder return "true" value to MediaOMXReader. 7. MediaOMXReader::ReadMetadata also checks "IsWaitingMediaResources()"function. 8. But "IsWaitingMediaResources()" value is false. (At this time, state is "CLIENT_STATE_RESOURCE_ASSIGNED") 9. So, MediaDecoderStateMachin::DecodeMetadata function does not call StartWaitForResources(); and return "NS_ERROR_FAILURE" It will be checked why "IsWaitingMediaResources()" value is changed during step5~step8. Since above step, video format is not set and sometime vp9 video file does not be listed up in video player. I will attach VP9 video file. Please confirm this problem. Thanks.

I have attached VP9 video file.

QA Wanted for branch checks.

The bug repros on Flame 2.1, Flame 2.0, Flame 1.4 and Buri 2.1 Actual result: The attached video clip does not appear in the video list when the video app is launched. 100% repro rate on Flame, about 60% repro on Buri. Flame 2.1 BuildID: 20140808091209 Gaia: c97d1b6c3094e854377b6affa5f46b8d4b7316ce Gecko: f40c3647561c Platform Version: 34.0a1 Firmware Version: V123 User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0 Flame 2.0 BuildID: 20140808063734 Gaia: 4c8c6569f2fded3c610cb6baf2e86355b1004653 Gecko: 1a895eb2b312 Platform Version: 32.0 Firmware Version: V123 User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0 Flame 1.4 BuildID: 20140808033135 Gaia: 2b2849a61cd38e909ed1c3e4586d104bc96f7001 Gecko: 931bf8651711 Platform Version: 30.0 Firmware Version: V123 User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0 Buri 2.1 BuildID: 20140808091209 Gaia: c97d1b6c3094e854377b6affa5f46b8d4b7316ce Gecko: f40c3647561c Platform Version: 34.0a1 Firmware Version: v1.2device.cfg User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

I think is not a regression and some codec we do not support. i am NI'ing : CJ to comment here and see if this is something e can support in future release.

ni slee

(In reply to bhavana bajaj [:bajaj] from comment #4) > I think is not a regression and some codec we do not support. i am NI'ing : > CJ to comment here and see if this is something e can support in future > release. Hi bajaj, From comment 0, it looks like the code fails at loading h/w codec. I will spend some time on this bug to see why it fails.

(In reply to Chris Kreinbring [:CKreinbring] from comment #3) > The bug repros on Flame 2.1, Flame 2.0, Flame 1.4 and Buri 2.1 > Actual result: The attached video clip does not appear in the video list > when the video app is launched. 100% repro rate on Flame, about 60% repro > on Buri. 60% looks strange. since from 1.4 (Bug 986381), currently V9 should not be able to be decoded since there should be no V9 OMX HW or SW decoder for B2G, especially for Buri. Hi Chris, How many video files in your phone? If there are many new video files, like mp4, HW codec may be occupied by other videos to generate thumbnail, you may need to wait a while. To simplify this test, it would be better to have only one V9 video file. To repeat this test, you may need to change the file name and push it to device again.

I discussed with Blake. * android version <= 15 ** android does not support vp8/vp9 ** gecko supports vp8/vp9 S/W codec * android version > 15 ** android supports vp8 S/W codec. ** for webm files, gecko passes the data to OMXCodec. OMXCodec will decides whether it should be decoded from S/W or H/W codec(if chip set has H/W codec). But there is one problem on this path, there is no fallback mechanism(bug 1000671). That's why flame always fails. But even we fix that problem, vp9 seems to have legal issues(bug 1026707). In short, before ICS(including ICS), vp8/vp9 can be decoded but we may have poor user experience. After JB(including JB), we can decode vp8 from OMXCodec.

VP9 is not official or committed support in B2G. Just fyi.

I have found same symptom using "gaia/apps/ftu/style/images/tutorial/Sheets.mp4" file. Please copy "Sheets.mp4" file to SDcard. And try reproduce like comment 0. If you try insert and remove SDcard repeatedly, you can find that sometime "Sheets.mp4" file is not listed up. Please confirm it.

I have attached "Sheets.mp4" file.

"Sheets.mp4" file is not vp8/vp9 file. Codec of this file is AVC. Comment 0 symptom is detected using "Sheets.mp4" file like comment 10. Thanks.

Suggest to change the title.

Steven - Do you know if we are supposed to support AVC?

Hi Jason, We support AVC and I can also play Sheets.mp4 on my flame. I will try the STR in comment 10.

Hi Jaemin, From the comments - VP9 is not an officially supported file type. Also reading your STR this seems to be aggressive testing: (In reply to Jaemin Song from comment #10) > I have found same symptom using > "gaia/apps/ftu/style/images/tutorial/Sheets.mp4" file. > Please copy "Sheets.mp4" file to SDcard. > And try reproduce like comment 0. > If you try insert and remove SDcard repeatedly, > you can find that sometime "Sheets.mp4" file is not listed up. > > Please confirm it. I feel this "repeatedly insert/remove SDcard" is not an appropriate use case. Steven, If you can repro this with sheets.mp4 OR if you feel there is actually a problem, please nominate this to 2.0? again.

(In reply to Wayne Chang [:wchang] from comment #16) > I feel this "repeatedly insert/remove SDcard" is not an appropriate use case. Sometime, this issue is occurred when scanning SDcard first time. So, video file was not showed in video player. "repeatedly insert/remove SDcard" step was just needed for reproducing issue well. Thanks.

Flame does not support hot plug so that I cannot reproduce this problem here. We have ONLY one Madai in Taipei. I can reproduce this problem on it. But I have no source code and have no way to debug it.

(In reply to StevenLee[:slee] from comment #18) > Flame does not support hot plug so that I cannot reproduce this problem > here. We have ONLY one Madai in Taipei. I can reproduce this problem on it. > But I have no source code and have no way to debug it. Could you check problem using Buri? According comment 3, Buri seems to support hot plug. Thanks.

I have found reproduce step on Flame. [Reproduce step on Flame] 1. For generating multiple "Sheets.mp4", please copy and paste "Sheets.mp4" file. 2. Change name of the copy file. (ex: "Sheets1.mp4, Sheets2.mp4,,,,,Sheets10.mp4".) 3. Copy these files to SD card. 4. Insert SDcard to device and execute video player. 5. Player will scan video file. You can find that some files were not listed up. Reproducible rate: 30% Thanks.

(In reply to Jaemin Song from comment #20) > I have found reproduce step on Flame. Hi Jaemin, Thanks for the information, I will try to reproduce it on my device.

I found one place that has problem. When decoding meta data, [1], MediaOmxReader could be busy, [2]. If the H/W resource is free after [2] returns, then the if check in [3] fails because of the resource is free. So that some resource, MediaInfo, is not set and fails in [4]. Benjamin, does it make sense? I tried to fix the above bug. But the problem still exists because mState is not in DECODER_STATE_DECODING_METADATA. I will discuss with Benjamin tomorrow. [1] http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#1905 [2] http://dxr.mozilla.org/mozilla-central/source/content/media/omx/MediaOmxReader.cpp#154 [3] http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#1910 [4] http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#1924

Comment 0 describe the root cause. The problem is that we can't call IsWaitingResources() arbitrary because there is another binder thread will change the result of IsWaitingResources(). For now there are 2 places call this function, one is StateMachine, another is MediaOmxReader. We should adjust the code flow only allow one place to call this function.

Comment on attachment 8484894 [details] [diff] [review] bug-1050667.v01.patch Review of attachment 8484894 [details] [diff] [review]: ----------------------------------------------------------------- lgtm :)

v01 patch only fix the asychronous state between MediaOmxReader and StateMachine, v02 also fix the same state between MediaOmxReader and OmxDecoder.

Same fix for MediaCoderReader.

Comment on attachment 8486329 [details] [diff] [review] bug-1050667-mediacodec.patch Review of attachment 8486329 [details] [diff] [review]: ----------------------------------------------------------------- It would be better to fix the logic flaw in MediaDecoderStateMachine directly instead of workaround in each MediaDecoderReader subclass. ::: content/media/omx/MediaCodecReader.h @@ +414,5 @@ > > android::sp<android::ALooper> mLooper; > android::sp<android::MetaData> mMetaData; > > + bool mIsWaitingResources; It is a little weird to solve this problem by adding temporary variables in MediaDecoderReaders: - It seems MediaDecoderReaders don't have any interaction or logic on this variable. - The original logic flaw of waiting resource still exists. We should fix the logic in the client side (ex. MediaDecoderStateMachine) instead of adding extra variables in all MediaDecoderReaders to handle such behavior. Otherwise, each MediaDecoderReader which has chances to wait for resources should add this kind of workaround logic/variables in it. Suppose a better way to solve this problem is to fix the logic flaw directly in MediaDecoderStateMachine::NotifyWaitingForResourcesStatusChanged() and MediaDecoderStateMachine::DecodeMetadata(). At least we can check whether MediaDecoderStateMachine::NotifyWaitingForResourcesStatusChanged() has been called before MediaDecoderStateMachine::DecodeMetadata() reacquires the monitor after calling mReader->ReadMetadata().

Comment on attachment 8486328 [details] [diff] [review] bug-1050667.v02.patch Review of attachment 8486328 [details] [diff] [review]: ----------------------------------------------------------------- Same comments as comment 28.

Comment on attachment 8487086 [details] [diff] [review] bug-1050667.v02.patch Review of attachment 8487086 [details] [diff] [review]: ----------------------------------------------------------------- When I implemented the IsWaitingMediaResources() I did not care about the threading :-( - [1] Consistent IsWaitingMediaResources() return value during a series of a task. - [2] Ensure that MediaDecoderStateMachine::NotifyWaitingForResourcesStatusChanged() is called in DECODER_STATE_WAIT_FOR_RESOURCES. To ensure [1] somewhere need to cache the value. MediaOmxReader seems a correct place to do it, though caching it at OmxDecoder seems not a good idea. The caching should be limit to one place. Otherwise, it could cause another regression. To do it OmxDecoder::TryLoad() might need to split to two function. [2] is also very important. Current patch seems not care about it. It seems necessary to add a code to ensure [2].

(In reply to Sotaro Ikeda [:sotaro] from comment #32) > Comment on attachment 8487086 [details] [diff] [review] > bug-1050667.v02.patch > > Review of attachment 8487086 [details] [diff] [review]: > ----------------------------------------------------------------- > > When I implemented the IsWaitingMediaResources() I did not care about the > threading :-( > - [1] Consistent IsWaitingMediaResources() return value during a series of a > task. > - [2] Ensure that > MediaDecoderStateMachine::NotifyWaitingForResourcesStatusChanged() is called > in DECODER_STATE_WAIT_FOR_RESOURCES. > > To ensure [1] somewhere need to cache the value. MediaOmxReader seems a > correct place to do it, though caching it at OmxDecoder seems not a good > idea. > The caching should be limit to one place. Otherwise, it could cause another > regression. To do it OmxDecoder::TryLoad() might need to split to two > function. > Replace the TryLoad function by AllocateMediaResources() and AfterWaitingMediaResources(). After split, the TryLoad function contains only AllocateMediaResources(), so I remove TryLoad function. > [2] is also very important. Current patch seems not care about it. It seems > necessary to add a code to ensure [2]. I add a new function DoNotifyWaitingForResourcesStatusChanged() in StateMachine, and dispatch the task to mDecodeTaskQueue. Ensure the new function is called after DecodeMetadata. When the DoNotifyWaitingForResourcesStatusChanged is called, either the StatMachine is in DECODER_STATE_WAIT_FOR_RESOURCES, or it had completed the DecodeMetadata.

How about simply use a different return value of MediaDecoderReader::ReadMetadata()[1] to indicate the waiting condition, so that MediaDecoderStateMachine can decide whether to call StartWaitForResources()[2] or not by itself without caching and asking the state of MediaDecoderReader subclass again? [1] http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#1918 [2] http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#1925

Comment on attachment 8487805 [details] [diff] [review] bug-1050667.v03.patch Review of attachment 8487805 [details] [diff] [review]: ----------------------------------------------------------------- The patch becomes very good. It is very close to end! The followings are comments to the patch. ::: content/media/MediaDecoderStateMachine.cpp @@ +1423,5 @@ > + RefPtr<nsIRunnable> task( > + NS_NewRunnableMethod(this, > + &MediaDecoderStateMachine::DoNotifyWaitingForResourcesStatusChanged)); > + mDecodeTaskQueue->Dispatch(task); > +} This implementation is very good! I forgot that current MediaStateMachine is driven by TaskQueue. By it, the notification handling is done serially. DoNotifyWaitingForResourcesStatusChanged() is always called after the series of IsWaitingMediaResources() checks. Current code does it as multi-threaded ways. This code remove the necessity of caching IsWaitingMediaResources() value. Then, current code does not need the caching related change. ::: content/media/omx/MediaOmxReader.cpp @@ +41,5 @@ > , mHasAudio(false) > , mVideoSeekTimeUs(-1) > , mAudioSeekTimeUs(-1) > , mSkipCount(0) > + , mIsWaitingResources(false) Caching is not necessary anymore. MediaOmxReader::ReadMetadata() have only one IsWaitingMediaResources() calls. And DoNotifyWaitingForResourcesStatusChanged() is always called after the series of IsWaitingMediaResources() check. ::: content/media/omx/OmxDecoder.h @@ +151,5 @@ > // extracts data from a container. > // Note: RTSP requires a custom extractor because it doesn't have a container. > bool Init(sp<MediaExtractor>& extractor); > > + bool AfterWaitingMediaResources(); It seems better that the function name says what it does. From the current name, it is not clear what the function does.

(In reply to Sotaro Ikeda [:sotaro] from comment #35) > Comment on attachment 8487805 [details] [diff] [review] > bug-1050667.v03.patch > > Review of attachment 8487805 [details] [diff] [review]: > ----------------------------------------------------------------- > > The patch becomes very good. It is very close to end! The followings are > comments to the patch. > > ::: content/media/MediaDecoderStateMachine.cpp > @@ +1423,5 @@ > > + RefPtr<nsIRunnable> task( > > + NS_NewRunnableMethod(this, > > + &MediaDecoderStateMachine::DoNotifyWaitingForResourcesStatusChanged)); > > + mDecodeTaskQueue->Dispatch(task); > > +} > > This implementation is very good! I forgot that current MediaStateMachine is > driven by TaskQueue. By it, the notification handling is done serially. > DoNotifyWaitingForResourcesStatusChanged() is always called after the series > of IsWaitingMediaResources() checks. Current code does it as multi-threaded > ways. > > This code remove the necessity of caching IsWaitingMediaResources() value. > Then, current code does not need the caching related change. > > ::: content/media/omx/MediaOmxReader.cpp > @@ +41,5 @@ > > , mHasAudio(false) > > , mVideoSeekTimeUs(-1) > > , mAudioSeekTimeUs(-1) > > , mSkipCount(0) > > + , mIsWaitingResources(false) > > Caching is not necessary anymore. MediaOmxReader::ReadMetadata() have only > one IsWaitingMediaResources() calls. And It prevent this bugs inconsistency problem in Comment 0.

Sorry, my comment of removing the caching of IsWaitingMediaResources() was wrong. MediaOmxReader::ReadMetadata() and MediaDecoderStateMachine::DecodeMetadata() needs to be consistent.

(In reply to Bruce Sun [:brsun] from comment #34) > How about simply use a different return value of > MediaDecoderReader::ReadMetadata()[1] to indicate the waiting condition, so > that MediaDecoderStateMachine can decide whether to call > StartWaitForResources()[2] or not by itself without caching and asking the > state of MediaDecoderReader subclass again? I do not think that using return value is a good idea. I want to prevent such usage as much as possible.

It seems better to update the cached IsWaitingMediaResources() also on DoNotifyWaitingForResourcesStatusChanged(). And use it's value to it's judgement like current gecko. From it's API definition, NotifyWaitingForResourcesStatusChanged() does not ensure the resource reservation success, just notify the status is changed. Though current implementation always ensure it. And without updating the cached IsWaitingMediaResources(), it's value continue to stay as incorrect value.

Sorry for my comments churn.

(In reply to Sotaro Ikeda [:sotaro] from comment #39) > > And without updating the cached IsWaitingMediaResources(), it's value > continue to stay as incorrect value. It happens only when DoNotifyWaitingForResourcesStatusChanged() does not deliver the reservation success. Current implementation actually does not have such case. But in future, it could be changed.

I re-think about Comment 39, it seems not a good idea to update ached IsWaitingMediaResources() by calling OMXCodecProxy::IsWaitingMediaResources(). Because MediaDecoderStateMachine is event driven. In Current implementation, DoNotifyWaitingForResourcesStatusChanged() always ensure the media resource acquire. And the cached value is always updated correctly by calling UpdateWaitingMediaResources() in MediaOmxReader::ReadMetadata(). This is a tricky part, it seems helpful if related code have a comment to talk about it. Therefore what your path attachment 8487805 [details] [diff] [review] do is better than my Comment 39. It seems better to add comment how mIsWaitingResources is updated and used at the following declaration of mIsWaitingResources. > bool mIsWaitingResources; Comment 41 is not related to current implementation. It can be ignored now. When it becomes necessary, we can change the code from concrete requirements of the change.

Then attachment 8487805 [details] [diff] [review] seems ok for me except one function name and comments.

Comment on attachment 8487805 [details] [diff] [review] bug-1050667.v03.patch Review+ if one function name is updated and some comments are added.

r=sotaro Add some comments, rename the function name. try server: https://tbpl.mozilla.org/?tree=Try&rev=9a4f6b5bb411 enable mediacodec: https://tbpl.mozilla.org/?tree=Try&rev=1f4484e1abd6

Same fix for MediaCodec.

Hi, both patches failed to apply cleanly with errors like: patching file content/media/omx/MediaOmxReader.cpp Hunk #1 FAILED at 36 Hunk #3 FAILED at 143 2 out of 3 hunks FAILED -- saving rejects to file content/media/omx/MediaOmxReader.cpp.rej patching file content/media/omx/MediaOmxReader.h Hunk #1 FAILED at 34 1 out of 2 hunks FAILED -- saving rejects to file content/media/omx/MediaOmxReader.h.rej patching file content/media/omx/OmxDecoder.h Hunk #1 succeeded at 148 with fuzz 1 (offset 0 lines). patch failed, unable to continue (try -v) could you look into this and maybe rebase against a current tree ? thanks!

Rebase version. Try server: https://tbpl.mozilla.org/?tree=Try&rev=9a5d6e7239d5

Rebase.

https://hg.mozilla.org/integration/b2g-inbound/rev/1193525f1f15 https://hg.mozilla.org/integration/b2g-inbound/rev/ed6f3ec5a71b

This has caused the tour in FTU app to fail. The tour uses videos to show the user around. STR: 1. Flash the first failing KK build to Flame 2. Load the FTU 3. Skip through to the last page of setup that has "Start Tour" button 4. Tap "start Tour" -> nothing happens Gaia-Rev d170091ba1b5597b05f37fb259f6b8eb02568798 Gecko-Rev https://hg.mozilla.org/integration/b2g-inbound/rev/ed6f3ec5a71b Build ID 20140919005353 Version 35.0a1 Device Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20140919.043100 FW-Date Fri Sep 19 04:31:11 EDT 2014 Bootloader L1TC10011800 First failing: https://hg.mozilla.org/integration/b2g-inbound/rev/ed6f3ec5a71b Last working: https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=c8dee1c9cc3d Tomcat, can we backout?

(In reply to Zac C (:zac) from comment #52) > Tomcat, can we backout? backedout in https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=2cd08d771e60

(In reply to Zac C (:zac) from comment #52) > This has caused the tour in FTU app to fail. The tour uses videos to show > the user around. > > STR: > 1. Flash the first failing KK build to Flame > 2. Load the FTU > 3. Skip through to the last page of setup that has "Start Tour" button > 4. Tap "start Tour" -> nothing happens I confirmed the symptom. I am going to investigate about the problem.

I found that MediaDecoderStateMachine::DecodeMetadata() has a problem. It checks IsWaitingMediaResources() before calling mReader->ReadMetadata(). The following change seems to fix the problem in Comment 52. ------------------------------------------------- nsresult MediaDecoderStateMachine::DecodeMetadata() { AssertCurrentThreadInMonitor(); NS_ASSERTION(OnDecodeThread(), "Should be on decode thread."); MOZ_ASSERT(mState == DECODER_STATE_DECODING_METADATA); DECODER_LOG("Decoding Media Headers"); - if (mReader->IsWaitingMediaResources()) { - StartWaitForResources(); - return NS_OK; - } - nsresult res; MediaInfo info; {

See bug 1065855 comment 24 which outlines why the IsWaitingMediaResources was added before the call to ReadMetadata. What problem does this trigger?

(In reply to cajbir (:cajbir) from comment #56) > See bug 1065855 comment 24 which outlines why the IsWaitingMediaResources > was added before the call to ReadMetadata. What problem does this trigger? Now the code flow is: StateMachine: step 1. mReader->IsWaitingMediaResources() step 2. mReader->ReadMetadata step 3. mReader->IsWaitingMediaResources() By apply my patch, I update the waiting status in step 2, and make the state consistent between step 2 and step 3. So the problem is: step 1. return false, because the MediaOmxReader is not be initialized. step 2. return true, because we are waiting for the HW resource. step 3. return true, because my patch synchronous the state with step 2. Then, we wait for the NotifyWaitingForResourcesStatusChanged change the state to do these three steps again. step 1. return true, go into wait_state!! This is wrong because my patch only update the waiting state in step 2.

(In reply to cajbir (:cajbir) from comment #56) > See bug 1065855 comment 24 which outlines why the IsWaitingMediaResources > was added before the call to ReadMetadata. What problem does this trigger? I did not recognized that IsWaitingMediaResources is also used other than gonk. And the meaning of medis resource is extended by MP4Reader::IsWaitingMediaResources().

In MediaOmxReader::IsWaitingMediaResources() case, it can always parse metadata and necessity of waiting becomes clear in the middile of mReader->ReadMetadata. In MP4Reader::IsWaitingMediaResources() case, it might not even get metadata.

attachment 8491333 [details] [diff] [review] adds the cache to keep MediaCodecReader::IsWaitingMediaResources() consistent during the series of the IsWaitingMediaResources() calls. But it restricts the "IsWaitingMediaResources() return value" update only within MediaOmxReader::ReadMetadata(). Therefore when MediaDecoderStateMachine::DecodeMetadata() is called second time, IsWaitingMediaResources() still return stale old value.

It seems that the problem seems to be fixed if "IsWaitingMediaResources()" is updated to latest state at the top of MediaDecoderStateMachine::DecodeMetadata(). It seems easily achieved if we add a preamble function's call at the top of MediaDecoderStateMachine::DecodeMetadata(). And implement(override) it at MediaCodecReader class. Same thing can be achieved to do same thing at MediaDecoderStateMachine::DoNotifyWaitingForResourcesStatusChanged() and change the state only when the resource is acquired like current gecko's implementation.

Dear Sotaro, Currently, attached patches is not applied on Mozilla git. Could you let me know when you apply these patches on Mozilla git? Thanks.

I am not assigned to this bug. To commit a fix, the problem in Comment 52 needs to be addressed. I am not sure about the progress.

I am going to create a fix of Comment 52 soon.

Fix problem of Comment 52 based on attachment 8491333 [details] [diff] [review].

bechen, is there another problem to fix this bug than Comment 52? Comment 52 could be fixed like attachment 8496897 [details] [diff] [review].

Comment on attachment 8496897 [details] [diff] [review] patch - Fix a problem of Comment 52 Review of attachment 8496897 [details] [diff] [review]: ----------------------------------------------------------------- It looks good, seems no more problem here. But I still don't understand why "Check IsWaitingMediaResources before ReadMetadata" can resolve the bug 1065855 comment 24.

(In reply to Benjamin Chen [:bechen] from comment #67) > > But I still don't understand why "Check IsWaitingMediaResources before > ReadMetadata" can resolve the bug 1065855 comment 24. Can you explain more about which part you do not understand?

(In reply to Sotaro Ikeda [:sotaro] from comment #68) > (In reply to Benjamin Chen [:bechen] from comment #67) > > > > But I still don't understand why "Check IsWaitingMediaResources before > > ReadMetadata" can resolve the bug 1065855 comment 24. > > Can you explain more about which part you do not understand? Under the original design, the IsWaitingMediaResources is called after ReadMetadata because the ReadMetadata will try to allocate the HW resources. But at bug 1065855 (MediaSourceReader), it checks IsWaitingMediaResources before ReadMetadata, that implies the HW allocation is not triggered by ReadMetadata, it is triggered before ReadMetadata. So if the HW resource allocation is triggered before ReadMetadata, How do we guarantee the MediaDecoderStateMachine::NotifyWaitingForResourcesStatusChanged for MediaSourceReader is called when StateMachine's state is DECODER_STATE_WAIT_FOR_RESOURCES ?

(In reply to Benjamin Chen [:bechen] from comment #69) > (In reply to Sotaro Ikeda [:sotaro] from comment #68) > > (In reply to Benjamin Chen [:bechen] from comment #67) > > > > > > But I still don't understand why "Check IsWaitingMediaResources before > > > ReadMetadata" can resolve the bug 1065855 comment 24. > > > > Can you explain more about which part you do not understand? > > Under the original design, the IsWaitingMediaResources is called after > ReadMetadata because the ReadMetadata will try to allocate the HW resources. > But at bug 1065855 (MediaSourceReader), it checks IsWaitingMediaResources > before ReadMetadata, that implies the HW allocation is not triggered by > ReadMetadata, it is triggered before ReadMetadata. > So if the HW resource allocation is triggered before ReadMetadata, How do we > guarantee the > MediaDecoderStateMachine::NotifyWaitingForResourcesStatusChanged for > MediaSourceReader is called when StateMachine's state is > DECODER_STATE_WAIT_FOR_RESOURCES ? It is the responsibility of the MediaSourceReader. If you think it is a problem, can you create a new bug for it? It is a different problem. Therefore we should not put off this bug fix because of it. And can you close this bug soon?

Changed to a correct component.

(In reply to Sotaro Ikeda [:sotaro] from comment #70) > (In reply to Benjamin Chen [:bechen] from comment #69) > > (In reply to Sotaro Ikeda [:sotaro] from comment #68) > > > (In reply to Benjamin Chen [:bechen] from comment #67) > > > > > > > > But I still don't understand why "Check IsWaitingMediaResources before > > > > ReadMetadata" can resolve the bug 1065855 comment 24. > > > > > > Can you explain more about which part you do not understand? > > > > Under the original design, the IsWaitingMediaResources is called after > > ReadMetadata because the ReadMetadata will try to allocate the HW resources. > > But at bug 1065855 (MediaSourceReader), it checks IsWaitingMediaResources > > before ReadMetadata, that implies the HW allocation is not triggered by > > ReadMetadata, it is triggered before ReadMetadata. > > So if the HW resource allocation is triggered before ReadMetadata, How do we > > guarantee the > > MediaDecoderStateMachine::NotifyWaitingForResourcesStatusChanged for > > MediaSourceReader is called when StateMachine's state is > > DECODER_STATE_WAIT_FOR_RESOURCES ? > > It is the responsibility of the MediaSourceReader. If you think it is a > problem, can you create a new bug for it? It is a different problem. > Therefore we should not put off this bug fix because of it. And can you > close this bug soon? Yes, I'll try to close it soon after push it to tryserver and do some manual tests. Thanks for your help.

try: https://tbpl.mozilla.org/?tree=Try&rev=eeb39daac6f9

try server: https://tbpl.mozilla.org/?tree=Try&rev=375919c8e19b

Rebase.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f36f3a13dd61 https://hg.mozilla.org/integration/mozilla-inbound/rev/539cf3404ad2

https://hg.mozilla.org/mozilla-central/rev/f36f3a13dd61 https://hg.mozilla.org/mozilla-central/rev/539cf3404ad2

Dear Benjamin, This issue is affect on v2.0. Because of this, this patch is also needed to reflect to v2.0 branch. Please reflect this patch to v2.0 branch? Thanks.

Benjamin, can you please uplift this to v2.1, as the issue here affects v2.1 too? Thanks!

(In reply to Benjamin Chen [:bechen] from comment #79) > Created attachment 8501569 [details] [diff] [review] > bug-1050667.b2g32v2.0.patch Review is seems to need for this patch. Please set the flag to review+. Thanks.

(In reply to Jaemin Song from comment #81) > (In reply to Benjamin Chen [:bechen] from comment #79) > > Created attachment 8501569 [details] [diff] [review] > > bug-1050667.b2g32v2.0.patch > > Review is seems to need for this patch. > Please set the flag to review+. There is no value in having the author set a review+ flag. If the branch patch is different enough from the m-c patch that it needs another review, then review needs to be done by someone other than the author.

(In reply to Benjamin Chen [:bechen] from comment #79) > Created attachment 8501569 [details] [diff] [review] > bug-1050667.b2g32v2.0.patch If review is done and patch is landed on v2.0. To cherry-pick code, please share link. Thanks.

(In reply to Viorela Ioia [:viorela] from comment #80) > Benjamin, can you please uplift this to v2.1, as the issue here affects v2.1 > too? > Thanks! Ok, I'll provide the b2g34_v2_1 patch after I clone the repository.

Comment on attachment 8501569 [details] [diff] [review] bug-1050667.b2g32v2.0.patch Uplift the patch to v2.0, and I think the patch doesn't need review again because the codebase are almost the same. [Approval Request Comment] Bug caused by (feature/regressing bug #): 1050667 User impact if declined: Some thumbnails/video might not be shown on b2g device. Testing completed: manual test on m-c. Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none

Comment on attachment 8504642 [details] [diff] [review] bug-1050667.b2g34v2.1.patch v2.1 patch. [Approval Request Comment] Bug caused by (feature/regressing bug #): 1050667 User impact if declined: Some thumbnails/video might not be shown on b2g device. Testing completed: manual test on m-c. Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none

(In reply to Benjamin Chen [:bechen] from comment #85) > Comment on attachment 8501569 [details] [diff] [review] > bug-1050667.b2g32v2.0.patch > > Uplift the patch to v2.0, and I think the patch doesn't need review again > because the codebase are almost the same. > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): 1050667 > User impact if declined: Some thumbnails/video might not be shown on b2g > device. > Testing completed: manual test on m-c. > Risk to taking this patch (and alternatives if risky): low > String or UUID changes made by this patch: none Dear Benjamin, When is approval flag(approval-mozilla-b2g32?) confirmed? Additionally, this patch seems to occur build error, because LOG code in "MediaDecodeStateMachine" is not proper. After build error is fixed and build success is confirmed, please commit this patch on v2.0 and share commit link. To cherry-pick this patch to our git, we need a commit link(2.0). Please check it ASAP. Thanks.

fix build error. [Approval Request Comment] Bug caused by (feature/regressing bug #): 1050667 User impact if declined: Some thumbnails/video might not be shown on b2g device. Testing completed: manual test on m-c. Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none

Hi Bobby, Can you help the approval procedure? Thanks/

Hi Bhavana, could you help approve this request?

(In reply to Jaemin Song from comment #0) > User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like > Gecko) Chrome/36.0.1985.125 Safari/537.36 > > Steps to reproduce: > > 1. Copy attached VP9 video file to SDcard. > 2. Insert SDcard to device and execute video player. > 3. Player will scan video file. > 4. If player success scanning, remove SDcard and insert SDcard again to scan > file. > 5. If you repeat step4, sometime VP9 video file is not listed up. > > > > Actual results: > > [Code level step of issue case] > 1. Player try to scan VP9 video file. > 2. During scaning, gecko Calls MediaDecoderStateMachine::DecodeMetadata > 3. In DecodeMetadata function, call MediaOMXReader::ReadMetadata. > 4. ReadMetadata calls "OmxDecoder::TryLoad()". > 5. But when issue is occurred, videoSouce(OMXCodecProxy.cpp) is waiting > resources. > (At this time, state is "CLIENT_STATE_WAIT_FOR_RESOURCE") > 6. So, to wait resources, OmxDecoder return "true" value to MediaOMXReader. > 7. MediaOMXReader::ReadMetadata also checks > "IsWaitingMediaResources()"function. > 8. But "IsWaitingMediaResources()" value is false. > (At this time, state is "CLIENT_STATE_RESOURCE_ASSIGNED") > 9. So, MediaDecoderStateMachin::DecodeMetadata function does not call > StartWaitForResources(); > and return "NS_ERROR_FAILURE" > > It will be checked why "IsWaitingMediaResources()" value is changed during > step5~step8. > Since above step, video format is not set and > sometime vp9 video file does not be listed up in video player. > I will attach VP9 video file. > Please confirm this problem. > > Thanks. Can you please help verify that the issue is fixed for once it lands on 2.0 ?

Comment on attachment 8507596 [details] [diff] [review] bug-1050667.b2g32v2.0.patch Approving this is blocking the partner release for 2.0 . Please make sure to verify this once it lands on all branches.

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/70cd37b5bcc3 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f92e54a91038

(In reply to bhavana bajaj [:bajaj] from comment #92) > (In reply to Jaemin Song from comment #0) > > User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like > > Gecko) Chrome/36.0.1985.125 Safari/537.36 > > > > Steps to reproduce: > > > > 1. Copy attached VP9 video file to SDcard. > > 2. Insert SDcard to device and execute video player. > > 3. Player will scan video file. > > 4. If player success scanning, remove SDcard and insert SDcard again to scan > > file. > > 5. If you repeat step4, sometime VP9 video file is not listed up. > > > > > > > > Actual results: > > > > [Code level step of issue case] > > 1. Player try to scan VP9 video file. > > 2. During scaning, gecko Calls MediaDecoderStateMachine::DecodeMetadata > > 3. In DecodeMetadata function, call MediaOMXReader::ReadMetadata. > > 4. ReadMetadata calls "OmxDecoder::TryLoad()". > > 5. But when issue is occurred, videoSouce(OMXCodecProxy.cpp) is waiting > > resources. > > (At this time, state is "CLIENT_STATE_WAIT_FOR_RESOURCE") > > 6. So, to wait resources, OmxDecoder return "true" value to MediaOMXReader. > > 7. MediaOMXReader::ReadMetadata also checks > > "IsWaitingMediaResources()"function. > > 8. But "IsWaitingMediaResources()" value is false. > > (At this time, state is "CLIENT_STATE_RESOURCE_ASSIGNED") > > 9. So, MediaDecoderStateMachin::DecodeMetadata function does not call > > StartWaitForResources(); > > and return "NS_ERROR_FAILURE" > > > > It will be checked why "IsWaitingMediaResources()" value is changed during > > step5~step8. > > Since above step, video format is not set and > > sometime vp9 video file does not be listed up in video player. > > I will attach VP9 video file. > > Please confirm this problem. > > > > Thanks. > > Can you please help verify that the issue is fixed for once it lands on 2.0 ? Yes. After applying the patch, I had confirmed that issue is fixed on 2.0. Thanks.

This issue is verified fixed on Flame 2.2 and 2.1. Result: The VP9 video file is listed and can be played properly on Video app, following STRs in Comment 0. Flame 2.2 Device: Flame Master (319mb)(Kitkat Base)(Full Flash) Build ID: 20141028040202 Gaia: 6a7fb482a03c5083ef79b41e7b0dfab27527cd04 Gecko: a255a234946e Version: 36.0a1 (Master) Firmware Version: v188 User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0 Flame 2.1 Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash) BuildID: 20141028001203 Gaia: a0174f7166745256aaca1cb3aa9f894033fbffa6 Gecko: 43bda3541f6b Gonk: 6e51d9216901d39d192d9e6dd86a5e15b0641a89 Version: 34.0 (2.1) Firmware: V188 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 ======================================= Leaving verifyme tag for 2.0M.

This bug has been verified to fail on Woodduck 2.0;can't find "Sheet.mp4" in Videos app only find "VP9.webm" in list. Reproducing rate: 5/5 Found time:15:13 around See attachment: Verify_Woodduck_Sheet.MP4,logcat_Verify_1513.txt build version: Gaia-Rev 87b23fa81c3b59f2ba24b84f7d686f4160d4e7cb Gecko-Rev d049d4ef127844121c9cf14d2e8ca91fd9045fcb Build-ID 20141124050313 Version 32.0

Hi Peipei, According to comment 98, verification result is fail. Could you please help check it on Woodduck again?

(In reply to Hubert Lu[:hlu] <hlu@mozilla.com> from comment #101) > Hi Peipei, > According to comment 98, verification result is fail. Could you please > help check it on Woodduck again? Hubert, I verified again on both white and black woodduck. The result on white woodduck is PASS. But the result on black woodduck is FAIL. So I think there are two possible reasons: 1. The build for black woodduck didn't include this fix. 2. Black woodduck hardware has issues. Black woodduck ---------------------------------------------------------------------------- Gaia-Rev d742e375aca6dc1bf3a36638000ad7f5338ef457 Gecko-Rev d049d4ef127844121c9cf14d2e8ca91fd9045fcb Build-ID 20141126050313 Version 32.0 Device-Name jrdhz72_w_ff FW-Release 4.4.2 FW-Incremental 1416949523 FW-Date Wed Nov 26 05:05:47 CST 2014 white woodduck build ----------------------------------------------------------------------------- Gaia-Rev a273ab9c18e9184eb02722b25c73e2ba7680cc09 Gecko-Rev e7df4dde2d9dbedee942333d34eaea2afe32bebc Build-ID 20141017100433 Version 32.0 Device-Name soul35 FW-Release 4.4.2 FW-Incremental 1413510704 FW-Date Fri Oct 17 09:52:15 CST 2014