Closed Bug 1069857 Opened 6 years ago Closed 6 years ago

[FFOS] OMXCodec's seek could be triggered again in DecodeVideoFrame() when seeking

Categories

(Core :: Audio/Video, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla35
blocking-b2g 2.0+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: bwu, Assigned: bwu)

References

Details

(Whiteboard: [2.0S+])

Attachments

(1 file, 1 obsolete file)

There is not a small chance to hit this problem if decoder takes longer time to get a first valid frame when seeking. In this case, mVideoSeekTimeUs will not be set -1 @http://dxr.mozilla.org/mozilla-central/source/content/media/omx/MediaOmxReader.cpp?from=MediaOmxReader.cpp&case=true#368 when time (MAX_VIDEO_DECODE_SECONDS) is expired, a new seek will be triggered to OmxDecoder again in MediaOmxReader::DecodeVideoFrame(). This new seek needs to be avoided. 

For some platforms, like woodduck, the first 1~2 decoded frames after seeking are invalid (length is 0). See Bug 1057172.
Clear mVideoSeekTimeUs in time. Otherwise when time is expired as comment 0 OmxDecoder will do the seek again.
Attachment #8493586 - Flags: review?(jwwang)
Comment on attachment 8493586 [details] [diff] [review]
Bug-1069857-clear-seek-timestamp-in-time.patch

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

Is it better to reset |mVideoSeekTimeUs| right after [1]? This ensures |mVideoSeekTimeUs| is reset even if decode fails at [2].

[1] http://hg.mozilla.org/mozilla-central/file/5e704397529b/content/media/omx/MediaOmxReader.cpp#l340
[2] http://hg.mozilla.org/mozilla-central/file/5e704397529b/content/media/omx/MediaOmxReader.cpp#l350
Attachment #8493586 - Flags: review?(jwwang)
(In reply to JW Wang [:jwwang] from comment #2)
> Comment on attachment 8493586 [details] [diff] [review]
> Bug-1069857-clear-seek-timestamp-in-time.patch
> 
> Review of attachment 8493586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is it better to reset |mVideoSeekTimeUs| right after [1]? This ensures
> |mVideoSeekTimeUs| is reset even if decode fails at [2].
If decode fails at [2], the video playback will be finished. So it doesn't matter to reset it since DecodeVideoFrame() should not be entered again.
Attachment #8493586 - Flags: review?(cpearce)
Attachment #8493586 - Flags: review?(cpearce) → review+
Carry reviewer, jwang and cpearce.
Attachment #8493586 - Attachment is obsolete: true
Attachment #8494282 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/408238add852
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
This patch is required in 2.0M branch.
blocking-b2g: --- → 2.0M?
Depends on: 1072947
per comment8, 2.0m+
blocking-b2g: 2.0M? → 2.0M+
Whiteboard: [2.0S+]
Should this be considered for 2.1 as well? If so, please nominate :)
[Blocking Requested - why for this release]:
1. Flame can be reproduced, generic bug.
2. partner-blocker

Triage:
Hi Ryan, This is partner-blocker
Please also apply the patch on 2.0 and 2.1. 
Thanks!
Blocks: Woodduck
blocking-b2g: 2.0M+ → 2.0+
Flags: needinfo?(ryanvm)
Whiteboard: [2.0S+] → [2.0S+][partner-blocker]
Josh, For bug 1054172, the patch is landed on v2.0m in comment 10. For 2.0 and 2.1, it needs to nominate for approval.
Hi Blake,
Could you please raise approval request for 2.0? 
Thanks!
Flags: needinfo?(bwu)
Comment on attachment 8494282 [details] [diff] [review]
Bug-1069857-clear-seek-timestamp-in-time.patch

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 #): 
NA. 
User impact if declined:
under some chances, seeking could take unexpectedly longer.  
Testing completed: 
Yes.
Risk to taking this patch (and alternatives if risky):
pretty low 
String or UUID changes made by this patch:
Attachment #8494282 - Flags: approval-mozilla-b2g32?
Flags: needinfo?(bwu)
Update for more info. 
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
None. 
User impact if declined:
Under some chances (the speed of decoding or seeking is slow), seeking could take unexpectedly longer.  
Testing completed: 
Yes. 
Risk to taking this patch (and alternatives if risky):
pretty low (no change for original design and it avoids unnecessary seeking under some chances)
String or UUID changes made by this patch:
None
Still needs an aurora approval request.
Flags: needinfo?(ryanvm) → needinfo?(bwu)
Attachment #8494282 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Thanks!
Flags: needinfo?(bwu)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> Still needs an aurora approval request.
Flags: needinfo?(bwu)
Comment on attachment 8494282 [details] [diff] [review]
Bug-1069857-clear-seek-timestamp-in-time.patch

Approval Request Comment
[Feature/regressing bug #]:
None.
[User impact if declined]:
Under some chances (the speed of decoding or seeking is slow), seeking could take unexpectedly longer.  
[Describe test coverage new/current, TBPL]:
1. https://tbpl.mozilla.org/?tree=Try&rev=8bd643492f5c (2.0)
2. Has landed in 2.0
[Risks and why]: 
Pretty low (no change for original design and it avoids unnecessary seeking under some chances)
[String/UUID change made/needed]:
None
Attachment #8494282 - Flags: approval-mozilla-aurora?
Flags: needinfo?(bwu)
Attachment #8494282 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [2.0S+][partner-blocker] → [2.0S+]
Blocks: Woodduck_P2
Priority: -- → P1
No longer blocks: Woodduck_P2
You need to log in before you can comment on or make changes to this bug.