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

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

Attachment: Bug-1069857-clear-seek-timestamp-in-time.patch

Clear mVideoSeekTimeUs in time. Otherwise when time is expired as comment 0 OmxDecoder will do the seek again.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 3

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

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

Comment 4

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

Attachment: Bug-1069857-clear-seek-timestamp-in-time.patch

Carry reviewer, jwang and cpearce.

Comment 5

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

TBPL: https://tbpl.mozilla.org/?tree=Try&rev=8bd643492f5c
All green!

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/408238add852

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/408238add852

Comment 8

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

This patch is required in 2.0M branch.

Comment 9

[Francis Lee [:frlee]]

per comment8, 2.0m+

Comment 10

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/aea28463e1ff

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Should this be considered for 2.1 as well? If so, please nominate :)

Comment 12

[Josh Cheng [:josh]]

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

Comment 13

[Kai-Zhen Li [:seinlin][:kli]]

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.

Comment 14

[Josh Cheng [:josh]]

Hi Blake,
Could you please raise approval request for 2.0? 
Thanks!

Comment 15

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

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:

Comment 16

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

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Still needs an aurora approval request.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e769e10ddd62

Comment 19

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

Thanks!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> Still needs an aurora approval request.

Comment 21

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

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/44c1345c9000