Closed Bug 1050667 Opened 5 years ago Closed 5 years ago

[MADAI][Multimedia] Sometimes MP4 video file is not listed up in video player.

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla35
blocking-b2g -
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v1.4 --- affected
b2g-v2.0 --- verified
b2g-v2.0M --- fixed
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: jaemin1.song, Assigned: bechen)

References

Details

(Keywords: verifyme, Whiteboard: [ LibGLA, TD106401, PP1 , B ])

Attachments

(8 files, 13 obsolete files)

175.08 KB, video/x-matroska
Details
203.66 KB, video/mp4
Details
4.48 KB, patch
bechen
: review+
Details | Diff | Splinter Review
13.13 KB, patch
bechen
: review+
Details | Diff | Splinter Review
13.03 KB, patch
Details | Diff | Splinter Review
13.02 KB, patch
Details | Diff | Splinter Review
3.90 MB, text/plain
Details
2.82 MB, video/mp4
Details
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.
blocking-b2g: --- → 2.0?
Whiteboard: [ LibGLA, dev , B ]
Attached video VP9.webm
I have attached VP9 video file.
QA Wanted for branch checks.
Component: General → Gaia::Video
Keywords: qawanted
QA Contact: ckreinbring
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
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.
blocking-b2g: 2.0? → backlog
Flags: needinfo?(cku)
ni slee
Flags: needinfo?(cku) → needinfo?(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.
Flags: needinfo?(slee)
(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.
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
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.
Attached video Sheets.mp4
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.
blocking-b2g: backlog → 2.0?
Suggest to change the title.
Summary: [MADAI][Multimedia] Sometime, Vp9 video file was not listed up in video player. → [MADAI][Multimedia] Sometimes MP4 video file is not listed up in video player.
Steven - Do you know if we are supposed to support AVC?
Flags: needinfo?(slee)
Hi Jason,

We support AVC and I can also play Sheets.mp4 on my flame. I will try the STR in comment 10.
Flags: needinfo?(slee)
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.
blocking-b2g: 2.0? → -
Flags: needinfo?(jaemin1.song)
(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.
Flags: needinfo?(jaemin1.song)
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.
Flags: needinfo?(slee)
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
Flags: needinfo?(slee) → needinfo?(bechen)
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.
Flags: needinfo?(bechen)
Assignee: nobody → bechen
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch bug-1050667.v01.patch (obsolete) — Splinter Review
Attachment #8484894 - Flags: feedback?(slee)
Attachment #8484894 - Flags: feedback?(brsun)
Comment on attachment 8484894 [details] [diff] [review]
bug-1050667.v01.patch

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

lgtm :)
Attachment #8484894 - Flags: feedback?(slee) → feedback+
Attached patch bug-1050667.v02.patch (obsolete) — Splinter Review
v01 patch only fix the asychronous state between MediaOmxReader and StateMachine, v02 also fix the same state between MediaOmxReader and OmxDecoder.
Attachment #8484894 - Attachment is obsolete: true
Attachment #8484894 - Flags: feedback?(brsun)
Attachment #8486328 - Flags: feedback?(brsun)
Attached patch bug-1050667-mediacodec.patch (obsolete) — Splinter Review
Same fix for MediaCoderReader.
Attachment #8486329 - Flags: feedback?(brsun)
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().
Attachment #8486329 - Flags: feedback?(brsun) → feedback-
Comment on attachment 8486328 [details] [diff] [review]
bug-1050667.v02.patch

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

Same comments as comment 28.
Attachment #8486328 - Flags: feedback?(brsun) → feedback-
Attached patch bug-1050667.v02.patch (obsolete) — Splinter Review
Attachment #8486328 - Attachment is obsolete: true
Attached patch bug-1050667-mediacodec.patch (obsolete) — Splinter Review
Attachment #8486329 - Attachment is obsolete: true
Attachment #8487086 - Flags: review?(sotaro.ikeda.g)
Attachment #8487087 - Flags: review?(sotaro.ikeda.g)
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].
Attachment #8487086 - Flags: review?(sotaro.ikeda.g)
Attachment #8487087 - Flags: review?(sotaro.ikeda.g)
Attached patch bug-1050667.v03.patch (obsolete) — Splinter Review
(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.
Attachment #8487086 - Attachment is obsolete: true
Attachment #8487805 - Flags: review?(sotaro.ikeda.g)
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
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(bechen)
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.
Attachment #8487805 - Flags: review?(sotaro.ikeda.g)
(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.
Flags: needinfo?(sotaro.ikeda.g)
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.
Attachment #8487805 - Flags: review+
Attached patch bug-1050667.v03.patch (obsolete) — Splinter Review
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
Attachment #8487805 - Attachment is obsolete: true
Attachment #8489839 - Flags: review+
Flags: needinfo?(bechen)
Attached patch bug-1050667-mediacodec.patch (obsolete) — Splinter Review
Same fix for MediaCodec.
Attachment #8487087 - Attachment is obsolete: true
Attachment #8489840 - Flags: review+
Duplicate of this bug: 1024517
Keywords: checkin-needed
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!
Flags: needinfo?(bechen)
Keywords: checkin-needed
Attached patch bug-1050667.v03.patch (obsolete) — Splinter Review
Rebase version.
Try server:
https://tbpl.mozilla.org/?tree=Try&rev=9a5d6e7239d5
Attachment #8489839 - Attachment is obsolete: true
Attachment #8491333 - Flags: review+
Flags: needinfo?(bechen)
Attached patch bug-1050667-mediacodec.patch (obsolete) — Splinter Review
Rebase.
Attachment #8489840 - Attachment is obsolete: true
Attachment #8491334 - Flags: review+
Keywords: checkin-needed
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?
Flags: needinfo?(cbook)
(In reply to Zac C (:zac) from comment #52)

> Tomcat, can we backout?

backedout in https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=2cd08d771e60
Flags: needinfo?(cbook)
(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 Also: → 1065855
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.
bechen, is there another problem to fix this bug than Comment 52? Comment 52 could be fixed like attachment 8496897 [details] [diff] [review].
Flags: needinfo?(bechen)
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.
Flags: needinfo?(bechen)
(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?
Flags: needinfo?(bechen)
(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 ?
Flags: needinfo?(bechen)
(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?
Flags: needinfo?(bechen)
Component: Gaia::Video → Video/Audio
Product: Firefox OS → Core
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.
Flags: needinfo?(bechen)
Attached patch bug-1050667.v04.patch (obsolete) — Splinter Review
try:
https://tbpl.mozilla.org/?tree=Try&rev=eeb39daac6f9
Attachment #8491333 - Attachment is obsolete: true
Attachment #8496897 - Attachment is obsolete: true
Attachment #8498656 - Flags: review+
Rebase.
Attachment #8498656 - Attachment is obsolete: true
Attachment #8500266 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f36f3a13dd61
https://hg.mozilla.org/mozilla-central/rev/539cf3404ad2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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.
Flags: needinfo?(bechen)
Whiteboard: [ LibGLA, dev , B ] → [ LibGLA, TD106401, PP1 , B ]
Attached patch bug-1050667.b2g32v2.0.patch (obsolete) — Splinter Review
Flags: needinfo?(bechen)
Benjamin, can you please uplift this to v2.1, as the issue here affects v2.1 too?
Thanks!
Flags: needinfo?(bechen)
(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.
Flags: needinfo?(bechen)
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
Attachment #8501569 - Flags: approval-mozilla-b2g32?
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
Attachment #8504642 - Flags: approval-mozilla-b2g34?
(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.
Flags: needinfo?(bechen)
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
Attachment #8501569 - Attachment is obsolete: true
Attachment #8501569 - Flags: approval-mozilla-b2g32?
Flags: needinfo?(bechen)
Attachment #8507596 - Flags: approval-mozilla-b2g32?
Hi Bobby,

Can you help the approval procedure?
Thanks/
Flags: needinfo?(bchien)
Hi Bhavana, could you help approve this request?
Flags: needinfo?(bbajaj)
Flags: needinfo?(bbajaj)
Attachment #8504642 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
(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 ?
Flags: needinfo?(jaemin1.song)
Keywords: verifyme
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.
Attachment #8507596 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
(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.
Flags: needinfo?(jaemin1.song)
Flags: needinfo?(bchien)
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+][lead-review+] → [QAnalyst-Triage?][lead-review+]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][lead-review+] → [QAnalyst-Triage+][lead-review+]
Flags: needinfo?(ktucker)
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
Flags: needinfo?(hlu)
Attached file logcat_Verify_1513.txt
Hi Peipei,
    According to comment 98, verification result is fail. Could you please help check it on Woodduck again?
Flags: needinfo?(hlu) → needinfo?(pcheng)
(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
Flags: needinfo?(pcheng)
You need to log in before you can comment on or make changes to this bug.