1059661 - [FFOS 2.0] MP4 Video rewind and forward operation don't work well for large GOP video files

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect)

Description

[vsireesha246]

STR:

Open video app->Play the attached video file->Play/seek upto 2.48sec of the video->Long press on the rewind button.

Actual:Long press on Rewind stops working.

Expected:It should work properly.

Comment 1

[vsireesha246]

This issue is reproducible on latest V2.0

Comment 2

[vsireesha246]

Attachment: MP4 My Sweetie.part2.rar

Comment 3

[vsireesha246]

Attachment: MP4 My Sweetie.part1.rar

Comment 4

[vsireesha246]

Attachment: MP4 My Sweetie.part3.rar

Comment 5

[Pi Wei Cheng [:piwei] (inactive)]

Unable to reproduce the issue. Using the attached video, the rewind button works as expected, whether it's long pressing or short pressing, the video rewinds as expected.

Tested on:
Device: Flame
BuildID: 20140902134703
Gaia: 449d8db9b3ea1f9262db822c37ef2143477172b7
Gecko: 13b41e22c8f6
Version: 32.0 (2.0)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 6

[vsireesha246]

Attachment: Logs_Bug_1059661

This issue logs are attached.

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

I had done some tests with this issue. This issue is able to reproduce with Firefox OS Simulator 2.0, but not in Firefox Nightly and Flame. According to reporter, it may be able to reproduce in madai.

Comment 8

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Cannot reproduce this with Flame + newest 2.0 as well(please refer to the following video). Please help to check if the gonk part porting is correct on your end

https://www.youtube.com/watch?v=c5jJp_VdXaU&feature=youtu.be

Thanks

Vance

Comment 9

[vsireesha246]

Hi Vance,

I can able to reproduce this issue in Nexsus4 with Mozilla V2.0 as well.
But in Nexsus4 the forward button having this problem.
Long/single press stops working.

If you need reproducing Video for Nexus i can able to provide you.

Thanks..
Sireesha

Comment 10

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to vsireesha246 from comment #9)
> Hi Vance,
> 
> I can able to reproduce this issue in Nexsus4 with Mozilla V2.0 as well.
> But in Nexsus4 the forward button having this problem.
> Long/single press stops working.
> 
> If you need reproducing Video for Nexus i can able to provide you.
> 
> Thanks..
> Sireesha

Hi Sireesha -

The fact that different device has different behavior(Flame works fine, Nexus4 only has problem with forward) indicates that this problem is probably BSP related. So we suggest you to check the bsp/decoder part first, and let us know if we can provide further help

Does that make sense to you?

Thanks

Comment 11

[Jaemin Song]

This issue is needed to register QCT SR.
Attached video file's mime type is "video/3g2".
So, player is using Qualcomm's extractor.
This problem is related to problem of Qualcomm's extractor.
I'll register this issue to QCT SR.

Thanks.

Comment 12

[vsireesha246]

Thank you Jaemin for your information about this issue.
As per the comment11 the issue is not in app side.So clearing NI.

Comment 13

[Jaemin Song]

I have found root cause.
And I have confirmed problem of forward seek on Flame.(v2.0)

In MADAI device, QCT extractor is using next seek mode.
(I don't know why QCT extractor is using next seek mode.)
Attached video file's I-Frame is on ...,77 sec, 106 sec ... etc. 
If I operate rewind seek at 106 sec ,
video player will seek to 96 sec.
But because there is no I-Frame on 96 sec,
position is moving to 106 sec.(Because next seek mode.)
So, when operating rewind seek at 106 sec,
rewind operation was not worked.

Similar issue was also occurred using native extractor.
As you know, gecko is using previous seek mode.
If video file's I-Frame interval is not constant like attached video file,
forward seek operation will be not worked.(Because previous seek mode.)
I have confirmed problem of forward seek on Flame.(v2.0)
I'll attach video file which is changed mime type to use native extractor.
You can find that forward seek is not worked.

Could you let me know whether this operation is normal?

Thanks.

Comment 14

[Jaemin Song]

Attachment: [Flame]MP4 My Sweetie.zip.001

To check this issue on Flame, I have attached video file.

Comment 15

[Jaemin Song]

Attachment: [Flame]MP4 My Sweetie.zip.002

Second file.

Comment 16

[Jaemin Song]

Dear Blake,

I don't know engineer who can answer about comment 13.
So, I request confirmation to you.
Can you answer about comment 13?
We need to check whether comment 13's operation is normal case.

Please confirm it.

Comment 17

[jongsoo.oh]

This issue might be a seek mode limiation according to comment 13
Could you verify the comment 13?

Comment 18

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

Jaemin, 
I cannot repro this problem according to comment 13 and comment 0 on my 2.0 Flame. 
Could you record a video to show us how you reproed this problem? 
Thanks!

Comment 19

[Jaemin Song]

Attachment: CAM02249.3gp

I have attached recorded video.
When you repro this issue,
please use [Flame]MP4 My Sweetie.mp4 file.

Thanks.

Comment 20

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

Thanks for your attachment 8494209 [details].
I can repro this problem now.

Comment 21

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

Confirmed the rootcause is as mentioned as comment 13. 
To fix this problem, I think we should seek to next I-frame when forward and seek to previosu I-frame when rewind.

Comment 22

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

Chens, 
Flame with master branch doesn't use fastSeek for rewind and forward, right?
I cannot repro this problem on master branch.

Comment 23

[Sherman Chen [:chens] (inactive)]

No, flame master also use fastSeek for rewind and forward, it's introduced in bug 996398.
For both seeking with slider or rewind/forward button, it always uses fastSeek to seek video.

[1]: https://github.com/mozilla-b2g/gaia/blob/165a1a70552466ee768fcac857b96b2e90e2745c/apps/video/js/forward_rewind_controller.js#L111
[2]: https://github.com/mozilla-b2g/gaia/blob/165a1a70552466ee768fcac857b96b2e90e2745c/apps/video/js/video.js#L1236

Comment 24

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

I found the reason why master branch doesn't have this problem. Bug 1022913, not fixed in 2.0, had some changes to make fastSeek to orignal seek when the behavior is not expected, like jump back when forward seek.

Comment 25

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

To fix this problem described in comment 13, I am going to modify the gecko codes to seek to next key frame for forward case and seek to previous key frame for rewind.

Comment 26

[Jaemin Song]

(In reply to Blake Wu [:bwu][:blakewu] from comment #25)
> To fix this problem described in comment 13, I am going to modify the gecko
> codes to seek to next key frame for forward case and seek to previous key
> frame for rewind.

Dear Blake,

We need to check current status for this issue.
Could you share the current progress status?

Thanks.

Comment 27

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

Attachment: Bug-1059661-Seek-to-next-keyframe-when-forward-other.patch

To fix this problem described in comment 13, seek to next key frame for forward case and seek to previous key frame for rewind.

Comment 28

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8500264 [details] [diff] [review]
Bug-1059661-Seek-to-next-keyframe-when-forward-other.patch

"Always use SEEK_PREVIOUS_SYN" was chosen for "before fast seek era(before bug 778077). If SEEK_PREVIOUS_SYN is chosen, we can correctly seeks to correct place even when there is no sync point until end of video.

But current b2g uses "fast seek" for seek, then the patch is correct. But by this choice, if there is not sync point until end of video, it seeks until end of video. It is a trade off.

Comment 29

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #28)
> Comment on attachment 8500264 [details] [diff] [review]
> Bug-1059661-Seek-to-next-keyframe-when-forward-other.patch
> 
> "Always use SEEK_PREVIOUS_SYN" was chosen for "before fast seek era(before
> bug 778077). If SEEK_PREVIOUS_SYN is chosen, we can correctly seeks to
> correct place even when there is no sync point until end of video.

If we do not want to lose this, we need to change how to seek depends on fast seek or not.

Comment 30

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8500264 [details] [diff] [review]
Bug-1059661-Seek-to-next-keyframe-when-forward-other.patch

Sorry, clear review flag based on comment 29.

Comment 31

[Sotaro Ikeda [:sotaro]]

cpearce, how to you think about comment 28 and comment 29?

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

That approach is acceptable.

Comment 33

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

(In reply to Sotaro Ikeda [:sotaro] from comment #28)
> Comment on attachment 8500264 [details] [diff] [review]
> Bug-1059661-Seek-to-next-keyframe-when-forward-other.patch
> 
> "Always use SEEK_PREVIOUS_SYN" was chosen for "before fast seek era(before
> bug 778077). If SEEK_PREVIOUS_SYN is chosen, we can correctly seeks to
> correct place even when there is no sync point until end of video.
> 
> But current b2g uses "fast seek" for seek, then the patch is correct. But by
> this choice, if there is not sync point until end of video, it seeks until
> end of video. It is a trade off.

Yes. Currently it is a tradeoff. With the MediaCodec(Bug 904177) design, we should be able to know if there is a next key frame or not. If no, we can seek to the previous one.

Comment 34

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

Most testing cases (https://treeherder.mozilla.org/ui/index.html#/jobs?repo=try&revision=e8ab9930b55c) look good but one test case failed (https://treeherder.mozilla.org/ui/index.html#/jobs?repo=try&revision=e8ab9930b55c) Need to modify the test case since the the original behavior (always seek to the previous keyframe) of seek  is changed.

Comment 35

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

attachment 8500264 [details] [diff] [review] only works for MP4 files. stagefright's webm extractor looks like not support to find next key frame.

Comment 36

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

Attachment: Bug-1059661-Remove-the-check-for-seeking-to-previous-keyframe-in-test-case.patch

Temporarily remove the check that if the timestamp of sought frame is smaller than seek time since originally we expected the seek is to jump to the previous key frame but the behavior of seek is changed as attachment 8500264 [details] [diff] [review]. 
For long-term, we should have a complete test case in fixing bug 1026330.

Comment 37

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8504088 [details] [diff] [review]
Bug-1059661-Remove-the-check-for-seeking-to-previous-keyframe-in-test-case.patch

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

::: content/media/test/test_fastSeek.html
@@ -50,5 @@
>        ok(v.currentTime >= keyframe - 0.05,
>           v.name + " seekTo=" + v.target + " currentTime (" + v.currentTime +
>           ") should be end up roughly after keyframe (" + keyframe + ") after fastSeek");
>  
> -      ok(v.currentTime <= v.target,

I think we should test that v.currentTime is less than the *next* keyframe. So something like:

var nextKeyframe = (v.keyframeIndex+1 < v.keyframes.length) ? v.keyframes[v.keyframeIndex+1] : v.duration;
ok(v.currentTime < nextKeyframe + 0.05, "....");

Comment 38

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

(In reply to Chris Pearce (:cpearce) from comment #37)
> Comment on attachment 8504088 [details] [diff] [review]
> Bug-1059661-Remove-the-check-for-seeking-to-previous-keyframe-in-test-case.
> patch
> 
> Review of attachment 8504088 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/test/test_fastSeek.html
> @@ -50,5 @@
> >        ok(v.currentTime >= keyframe - 0.05,
> >           v.name + " seekTo=" + v.target + " currentTime (" + v.currentTime +
> >           ") should be end up roughly after keyframe (" + keyframe + ") after fastSeek");
> >  
> > -      ok(v.currentTime <= v.target,
> 
> I think we should test that v.currentTime is less than the *next* keyframe.
> So something like:
> 
> var nextKeyframe = (v.keyframeIndex+1 < v.keyframes.length) ?
> v.keyframes[v.keyframeIndex+1] : v.duration;
> ok(v.currentTime < nextKeyframe + 0.05, "....");
Thanks for your review! 
I will modify it but only for MP4 files since other formats fail to jump to next keyframe as comment 35.

Comment 39

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #38)
> I will modify it but only for MP4 files since other formats fail to jump to
> next keyframe as comment 35.

But do they jump to a time less than the next keyframe? I think they would, so the test would still pass.

Comment 40

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

No. You can check the webm case in https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=361f9f747cb3. It jump to the previous key frame.  
Stagefright's demuxer doesn't support the seek to next key frame. If we can have a our own common demuxer, it would be easier to modify.

Comment 41

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

Attachment: Bug-1059661-Change-the-fastSeek-test-case-for-mp4-file.patch

Per discussion as comment 37 and 38. 
Test result: 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fa058987364d

Comment 42

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8504515 [details] [diff] [review]
Bug-1059661-Change-the-fastSeek-test-case-for-mp4-file.patch

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

::: content/media/test/test_fastSeek.html
@@ +51,5 @@
>           v.name + " seekTo=" + v.target + " currentTime (" + v.currentTime +
>           ") should be end up roughly after keyframe (" + keyframe + ") after fastSeek");
> +      // Bug 1059661 changed mp4 file to seek to next key frame when forward and seek to
> +      // the previous one when rewind.
> +      if (v.name.match(/.mp4/gi) == ".mp4") {

Your comment is incorrect. Only the behaviour on B2G was changed, not the behaviour on other platforms.

Remove the comment, and the else statement and the test contained in it. Testing that the time after seek is before the next keyframe is sufficient, as this is valid behaviour as per the spec.

Comment 43

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Pearce (:cpearce) from comment #39)
> But do they jump to a time less than the next keyframe?

(In reply to Blake Wu [:bwu][:blakewu] from comment #40)
> No. [...] It jump to the previous key frame.

Does the previous keyframe have a time less than the next keyframe?

Comment 44

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

(In reply to Chris Pearce (:cpearce) from comment #43)
> (In reply to Chris Pearce (:cpearce) from comment #39)
> > But do they jump to a time less than the next keyframe?
> 
> (In reply to Blake Wu [:bwu][:blakewu] from comment #40)
> > No. [...] It jump to the previous key frame.
> 
> Does the previous keyframe have a time less than the next keyframe?
Yes. Normally the previous keyframe have a time less than the next keyframe.

Comment 45

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

(In reply to Chris Pearce (:cpearce) from comment #42)
> Comment on attachment 8504515 [details] [diff] [review]
> Bug-1059661-Change-the-fastSeek-test-case-for-mp4-file.patch
> 
> Review of attachment 8504515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/test/test_fastSeek.html
> @@ +51,5 @@
> >           v.name + " seekTo=" + v.target + " currentTime (" + v.currentTime +
> >           ") should be end up roughly after keyframe (" + keyframe + ") after fastSeek");
> > +      // Bug 1059661 changed mp4 file to seek to next key frame when forward and seek to
> > +      // the previous one when rewind.
> > +      if (v.name.match(/.mp4/gi) == ".mp4") {
> 
> Your comment is incorrect. Only the behaviour on B2G was changed, not the
> behaviour on other platforms.
> 
> Remove the comment, and the else statement and the test contained in it.
> Testing that the time after seek is before the next keyframe is sufficient,
> as this is valid behaviour as per the spec.
Got your point via IRC. Except the incorrect comment, I thought the testing condition should be strict :)

Comment 46

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

Attachment: Bug-1059661-Change-the-fastSeek-test-case.patch

Per comment 42.

Comment 47

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8505222 [details] [diff] [review]
Bug-1059661-Change-the-fastSeek-test-case.patch

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

Strict enough. Thanks.

Comment 48

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

Attachment: Bug-1059661-Change-the-fastSeek-test-case-v2.patch

Carry r+ reviewer, cpearce.

Comment 49

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

Attachment: Bug-1059661-Seek-to-next-keyframe-when-forward-otherwise-seek-to-the-previous-one.patch

Carry r+ and reviewer, sotaro.

Comment 50

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

Hi Jaemin,
Is this bug required to fix in 2.0? 
If yes, we need to nominate it to be 2.0+.

Comment 51

[Jaemin Song]

(In reply to Blake Wu [:bwu][:blakewu] from comment #50)
> Hi Jaemin,
> Is this bug required to fix in 2.0? 
> If yes, we need to nominate it to be 2.0+.

Yes. This patch is required to fix in 2.0.

Thanks.

Comment 52

[Jaemin Song]

Dear Blake,

After applying your patch,
I detected side issue.

While playing attached file([Flame]MP4 My Sweetie.mp4),
if I seek to 3:20 sec, video frame is not updated.

In my opinion, if position of seek is bigger than "last I-frame",
player should seek using "SEEK_PREVIOUS_SYNC" option.

Please confirm this side issue.

Thanks.

Comment 53

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

(In reply to Jaemin Song from comment #52)
> Dear Blake,
> 
> After applying your patch,
> I detected side issue.
> 
> While playing attached file([Flame]MP4 My Sweetie.mp4),
> if I seek to 3:20 sec, video frame is not updated.
This is because there is no next key frame, so no more video frames are to be displayed. 
> 
> In my opinion, if position of seek is bigger than "last I-frame",
> player should seek using "SEEK_PREVIOUS_SYNC" option.
It is a good idea as mentioned in comment 33 as well. But I am afraid that behavior may violate WHATWG spec as discussed in Bug 1022913. so what we could do is to do accurate seeking when there is no next key frame. 

cpearce, 
May I have your opinion?
Thanks!

Comment 54

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

[Blocking Requested - why for this release]:
According to comment 51.

Comment 55

[Andrew Overholt [:overholt]]

Okay, 2.0+.  You'll also need uplift approval on the patch.

Comment 56

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

Attachment: Bug-1059661-Jump-to-the-previous-key-frame-and-do-accureate-seek-when no-next-key-frame

1. If EOS when seek to next key frame, jump to the previous key frame.
2. Do accurate seek (extracted from the patch in Bug 1022913)

Comment 57

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

Hi Jaemin, 
Could you have a try with the patches (attachment 8505358 [details] [diff] [review] and attachment 8506600 [details] [diff] [review])?

Comment 58

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #53)
> (In reply to Jaemin Song from comment #52)
> > Dear Blake,
> > 
> > After applying your patch,
> > I detected side issue.
> > 
> > While playing attached file([Flame]MP4 My Sweetie.mp4),
> > if I seek to 3:20 sec, video frame is not updated.
> This is because there is no next key frame, so no more video frames are to
> be displayed. 
> > 
> > In my opinion, if position of seek is bigger than "last I-frame",
> > player should seek using "SEEK_PREVIOUS_SYNC" option.
> It is a good idea as mentioned in comment 33 as well. But I am afraid that
> behavior may violate WHATWG spec as discussed in Bug 1022913. so what we
> could do is to do accurate seeking when there is no next key frame. 

That behaviour does not violate the spec. But it may be confusing, so doing accurate seek is OK, but slow, so we may be better to just not seek.

Comment 59

[Jaemin Song]

(In reply to Blake Wu [:bwu][:blakewu] from comment #57)
> Hi Jaemin, 
> Could you have a try with the patches (attachment 8505358 [details] [diff] [review]
> [review] and attachment 8506600 [details] [diff] [review])?

After applying these patches,
when position of seek is bigger than "last I-frame",
I can confirm that player doing "accurate seek" like you mentioned at comment 56.
Side issue is resolved.

Thanks.

Comment 60

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

(In reply to Chris Pearce (:cpearce) from comment #58)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #53)
> > > 
> > > In my opinion, if position of seek is bigger than "last I-frame",
> > > player should seek using "SEEK_PREVIOUS_SYNC" option.
> > It is a good idea as mentioned in comment 33 as well. But I am afraid that
> > behavior may violate WHATWG spec as discussed in Bug 1022913. so what we
> > could do is to do accurate seeking when there is no next key frame. 
> 
> That behaviour does not violate the spec. But it may be confusing, so doing
IIUC, as WHATWG spec (https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-fastseek) mentioned in item#9, 
"If the approximate-for-speed flag is set, adjust the new playback position to a value that will allow for playback to resume promptly. If new playback position before this step is before current playback position, then the adjusted new playback position must also be before the current playback position." 
Jump to the last key frame may violate the spec if the current playback position is "after" it. It will not violate the spec if the last key frame is after the current playback position.  
> accurate seek is OK, but slow, so we may be better to just not seek.

Comment 61

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8506600 [details] [diff] [review]
Bug-1059661-Jump-to-the-previous-key-frame-and-do-accureate-seek-when no-next-key-frame

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

r+ for MDSM change, since it's just uplifting code from bug 1022913. Sotaro needs to review the OmxDecoder change.

Comment 62

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8506600 [details] [diff] [review]
Bug-1059661-Jump-to-the-previous-key-frame-and-do-accureate-seek-when no-next-key-frame

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

Basically good except the following comment :-)

::: content/media/omx/OmxDecoder.cpp
@@ +816,5 @@
> +      // If there is no next Keyframe, jump to the previous key frame.
> +      if (err == ERROR_END_OF_STREAM && seekMode == MediaSource::ReadOptions::SEEK_NEXT_SYNC) {
> +         seekMode = MediaSource::ReadOptions::SEEK_PREVIOUS_SYNC;
> +	 findNextBuffer = true;
> +	 mIsVideoSeeking = true;

When changing "mIsVideoSeeking", need to hold mSeekLock.

Comment 63

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

(In reply to Sotaro Ikeda [:sotaro] from comment #62)
> Comment on attachment 8506600 [details] [diff] [review]
> Bug-1059661-Jump-to-the-previous-key-frame-and-do-accureate-seek-when
> no-next-key-frame
> 
> Review of attachment 8506600 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Basically good except the following comment :-)
> 
> ::: content/media/omx/OmxDecoder.cpp
> @@ +816,5 @@
> > +      // If there is no next Keyframe, jump to the previous key frame.
> > +      if (err == ERROR_END_OF_STREAM && seekMode == MediaSource::ReadOptions::SEEK_NEXT_SYNC) {
> > +         seekMode = MediaSource::ReadOptions::SEEK_PREVIOUS_SYNC;
> > +	 findNextBuffer = true;
> > +	 mIsVideoSeeking = true;
> 
> When changing "mIsVideoSeeking", need to hold mSeekLock.
Yeah. Thanks for the reminder!

Comment 64

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

Attachment: Bug-1059661-Jump-to-the-previous-key-frame-and-do-accurate-seek-v2.patch

1. Add mutex protection when changing mIsVideoSeeking as comment 62. 
2. Remove android log in MDSM added in attachment 8506600 [details] [diff] [review].

Comment 65

[Jaemin Song]

(In reply to Blake Wu [:bwu][:blakewu] from comment #64)
> Created attachment 8508426 [details] [diff] [review]
> Bug-1059661-Jump-to-the-previous-key-frame-and-do-accurate-seek-v2.patch
> 
> 1. Add mutex protection when changing mIsVideoSeeking as comment 62. 
> 2. Remove android log in MDSM added in attachment 8506600 [details] [diff] [review]
> [review].

Dear Blake,

If review is done,
please land these patches on 2.0.

Thanks.

Comment 66

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a52b662c714a
Test results look reasonable compared to the test cases in mozilla-b2g32_v2_0 (https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-b2g32_v2_0)

Comment 67

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

Attachment: Bug-1059661-Jump-to-the-previous-key-frame-and-do-accurate-seek-final.patch

Carry r+ from sotaro and cpearce.

Comment 68

[Ryan VanderMeulen [:RyanVM]]

I don't see any b2g32 approvals on these patches?

Comment 69

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

Comment on attachment 8505350 [details] [diff] [review]
Bug-1059661-Change-the-fastSeek-test-case-v2.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 #): 
None
User impact if declined:
Test case fails since the behavior of seek is changed in this bug.  
Testing completed: 
on TryServer
Risk to taking this patch (and alternatives if risky): 
Low (no alternative)
String or UUID changes made by this patch:
None

Comment 70

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

Comment on attachment 8505358 [details] [diff] [review]
Bug-1059661-Seek-to-next-keyframe-when-forward-otherwise-seek-to-the-previous-one.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 #): 
Feature Bug 778077 
User impact if declined: 
Seek forward and rewind may not work in those video files with big GOP.
Testing completed: 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a52b662c714a
Risk to taking this patch (and alternatives if risky): 
Low. 
String or UUID changes made by this patch:
None

Comment 71

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

Comment on attachment 8509330 [details] [diff] [review]
Bug-1059661-Jump-to-the-previous-key-frame-and-do-accurate-seek-final.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 #): 
Attachment 8505358 [details] [diff]
User impact if declined: 
With the patch (attachment 8505358 [details] [diff] [review]), if user seek to the position that is behind the last key frame. Video will be end of stream and not updated. 
Testing completed: 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a52b662c714a
Risk to taking this patch (and alternatives if risky): 
Low. 
String or UUID changes made by this patch:
None

Comment 72

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #68)
> I don't see any b2g32 approvals on these patches?
Sorry. My bad! I forgot to request uplift approvals.

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f3e077759f9f
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/adea59d09849
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/124f0bed1700

Comment 74

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

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/f3e077759f9f
http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/adea59d09849
http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/124f0bed1700

Comment 75

[Shine(Leave from Mozilla)]

Attachment: video