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

RESOLVED FIXED in Firefox 34

Status

()

defect
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bwu, Assigned: bwu)

Tracking

unspecified
mozilla35
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [2.0S+])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Blocks: 1068509
Assignee

Comment 1

5 years ago
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)
Assignee

Comment 3

5 years ago
(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.
Assignee

Updated

5 years ago
Attachment #8493586 - Flags: review?(cpearce)
Attachment #8493586 - Flags: review?(cpearce) → review+
Assignee

Comment 4

5 years ago
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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Assignee

Comment 8

5 years ago
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)
Assignee

Comment 15

5 years ago
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)
Assignee

Comment 16

5 years ago
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+
Assignee

Comment 19

5 years ago
Thanks!
Flags: needinfo?(bwu)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> Still needs an aurora approval request.
Flags: needinfo?(bwu)
Assignee

Comment 21

5 years ago
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+

Updated

5 years ago
Whiteboard: [2.0S+][partner-blocker] → [2.0S+]

Updated

5 years ago
Blocks: 1080481

Updated

5 years ago
Blocks: Woodduck_P2

Updated

5 years ago
Priority: -- → P1

Updated

5 years ago
No longer blocks: Woodduck_P2
You need to log in before you can comment on or make changes to this bug.