vsireesha246 Reporter
	Description • 10 years ago

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.

	vsireesha246 Reporter
	Comment 1 • 10 years ago

This issue is reproducible on latest V2.0

	Pi Wei Cheng [:piwei] (inactive)
	Comment 5 • 10 years ago

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

	vsireesha246 Reporter
	Comment 6 • 10 years ago

This issue logs are attached.

	John Hu [:johnhu][:johu][:醬糊小弟]
	Comment 7 • 10 years ago

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.

	Vance Chen [:vchen][skype:vance.lucida][vchen@mozilla.com]
	Comment 8 • 10 years ago

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

	vsireesha246 Reporter
	Comment 9 • 10 years ago

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

	Vance Chen [:vchen][skype:vance.lucida][vchen@mozilla.com]
	Comment 10 • 10 years ago

(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

	Jaemin Song
	Comment 11 • 10 years ago

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.

	vsireesha246 Reporter
	Comment 12 • 10 years ago

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

	Jaemin Song
	Comment 13 • 10 years ago

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.

	Jaemin Song
	Comment 14 • 10 years ago

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

	Jaemin Song
	Comment 15 • 10 years ago

Second file.

	Jaemin Song
	Comment 16 • 10 years ago

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.

	jongsoo.oh
	Comment 17 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 18 • 10 years ago

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!

	Jaemin Song
	Comment 19 • 10 years ago

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

Thanks.

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 20 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 21 • 10 years ago

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.

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 22 • 10 years ago

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

	Sherman Chen [:chens] (inactive)
	Comment 23 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 24 • 10 years ago

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.

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 25 • 10 years ago

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.

	Jaemin Song
	Comment 26 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 27 • 10 years ago

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

	Sotaro Ikeda [:sotaro]
	Comment 28 • 10 years ago

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.

	Sotaro Ikeda [:sotaro]
	Comment 29 • 10 years ago

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

	Sotaro Ikeda [:sotaro]
	Comment 30 • 10 years ago

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.

	Sotaro Ikeda [:sotaro]
	Comment 31 • 10 years ago

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

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 32 • 10 years ago

That approach is acceptable.

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 33 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 34 • 10 years ago

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.

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 35 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 36 • 10 years ago

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.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 37 • 10 years ago

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, "....");

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 38 • 10 years ago

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

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 39 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 40 • 10 years ago

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.

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 41 • 10 years ago

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

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 42 • 10 years ago

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.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 43 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 44 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 45 • 10 years ago

(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 :)

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 46 • 10 years ago

Per comment 42.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 47 • 10 years ago

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.

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 48 • 10 years ago

Carry r+ reviewer, cpearce.

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 49 • 10 years ago

Carry r+ and reviewer, sotaro.

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 50 • 10 years ago

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

	Jaemin Song
	Comment 51 • 10 years ago

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

	Jaemin Song
	Comment 52 • 10 years ago

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.

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 53 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 54 • 10 years ago

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

	Andrew Overholt [:overholt]
	Comment 55 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 56 • 10 years ago

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)

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 57 • 10 years ago

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

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 58 • 10 years ago

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

	Jaemin Song
	Comment 59 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 60 • 10 years ago

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

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 61 • 10 years ago

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.

	Sotaro Ikeda [:sotaro]
	Comment 62 • 10 years ago

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.

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 63 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 64 • 10 years ago

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

	Jaemin Song
	Comment 65 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 66 • 10 years ago

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)

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 67 • 10 years ago

Carry r+ from sotaro and cpearce.

	Ryan VanderMeulen [:RyanVM]
	Comment 68 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 69 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 70 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 71 • 10 years ago

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

	Blake Wu [:bwu][:blakewu] Assignee
	Comment 72 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 73 • 10 years ago

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

	Kai-Zhen Li [:seinlin][:kli]
	Comment 74 • 10 years ago

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