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

RESOLVED FIXED in 2.1 S8 (7Nov)

Status

defect
--
major
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: vsireesha246, Assigned: bwu)

Tracking

unspecified
2.1 S8 (7Nov)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.0M verified, b2g-v2.1 unaffected, b2g-v2.2 unaffected)

Details

(Whiteboard: [LibGLA,TD92155,QE1, A])

Attachments

(11 attachments, 6 obsolete attachments)

5.00 MB, application/x-rar
Details
5.00 MB, application/x-rar
Details
4.08 MB, application/x-rar
Details
353.28 KB, text/x-vhdl
Details
8.00 MB, application/zip
Details
6.21 MB, application/octet-stream
Details
236.43 KB, video/3gpp
Details
1.59 KB, patch
bwu
: review+
Details | Diff | Splinter Review
3.27 KB, patch
bwu
: review+
Details | Diff | Splinter Review
4.33 KB, patch
bwu
: review+
Details | Diff | Splinter Review
5.48 MB, video/mp4
Details
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.
This issue is reproducible on latest V2.0
Whiteboard: [LibGLA,TD92155,QE1, A]
Keywords: qawanted
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
Posted file Logs_Bug_1059661
This issue logs are attached.
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.
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
Flags: needinfo?(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
Flags: needinfo?(vsireesha246) → needinfo?(vchen)
(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
Flags: needinfo?(vchen) → needinfo?(vsireesha246)
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.
Thank you Jaemin for your information about this issue.
As per the comment11 the issue is not in app side.So clearing NI.
Flags: needinfo?(vsireesha246)
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.
Flags: needinfo?
To check this issue on Flame, I have attached video file.
Second file.
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.
Flags: needinfo?(bwu)
This issue might be a seek mode limiation according to comment 13
Could you verify the comment 13?
Flags: needinfo? → needinfo?(wchang)
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!
Flags: needinfo?(wchang)
Flags: needinfo?(jaemin1.song)
Flags: needinfo?(bwu)
Posted video CAM02249.3gp
I have attached recorded video.
When you repro this issue,
please use [Flame]MP4 My Sweetie.mp4 file.

Thanks.
Flags: needinfo?(jaemin1.song) → needinfo?(bwu)
Thanks for your attachment 8494209 [details].
I can repro this problem now.
Flags: needinfo?(bwu)
Summary: [Video]Rewind operation is not working for attached test content → [Video] Rewind and forward operation don't work well for large GOP video files
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.
Assignee: nobody → bwu
Chens, 
Flame with master branch doesn't use fastSeek for rewind and forward, right?
I cannot repro this problem on master branch.
Flags: needinfo?(shchen)
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
Flags: needinfo?(shchen)
Summary: [Video] Rewind and forward operation don't work well for large GOP video files → [FFOS 2.0] Video rewind and forward operation don't work well for large GOP video files
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.
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.
(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.
To fix this problem described in comment 13, seek to next key frame for forward case and seek to previous key frame for rewind.
Attachment #8500264 - Flags: review?(sotaro.ikeda.g)
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.
Attachment #8500264 - Flags: review?(sotaro.ikeda.g) → review+
(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 on attachment 8500264 [details] [diff] [review]
Bug-1059661-Seek-to-next-keyframe-when-forward-other.patch

Sorry, clear review flag based on comment 29.
Attachment #8500264 - Flags: review+
cpearce, how to you think about comment 28 and comment 29?
Flags: needinfo?(cpearce)
That approach is acceptable.
Flags: needinfo?(cpearce)
(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.
Attachment #8500264 - Flags: review+
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.
attachment 8500264 [details] [diff] [review] only works for MP4 files. stagefright's webm extractor looks like not support to find next key frame.
Summary: [FFOS 2.0] Video rewind and forward operation don't work well for large GOP video files → [FFOS 2.0] MP4 Video rewind and forward operation don't work well for large GOP video files
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.
Attachment #8504088 - Flags: review?(cpearce)
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, "....");
Attachment #8504088 - Flags: review?(cpearce) → review-
(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.
(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.
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.
Per discussion as comment 37 and 38. 
Test result: 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fa058987364d
Attachment #8504088 - Attachment is obsolete: true
Attachment #8504515 - Flags: review?(cpearce)
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.
Attachment #8504515 - Flags: review?(cpearce) → review-
(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?
(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.
(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 :)
Per comment 42.
Attachment #8504515 - Attachment is obsolete: true
Attachment #8505222 - Flags: review?(cpearce)
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.
Attachment #8505222 - Flags: review?(cpearce) → review+
Carry r+ reviewer, cpearce.
Attachment #8505222 - Attachment is obsolete: true
Attachment #8505350 - Flags: review+
Carry r+ and reviewer, sotaro.
Attachment #8500264 - Attachment is obsolete: true
Attachment #8505358 - Flags: review+
Hi Jaemin,
Is this bug required to fix in 2.0? 
If yes, we need to nominate it to be 2.0+.
Flags: needinfo?(jaemin1.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.
Flags: needinfo?(jaemin1.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.
Flags: needinfo?(bwu)
(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!
Flags: needinfo?(bwu) → needinfo?(cpearce)
[Blocking Requested - why for this release]:
According to comment 51.
blocking-b2g: --- → 2.0?
Okay, 2.0+.  You'll also need uplift approval on the patch.
blocking-b2g: 2.0? → 2.0+
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)
Hi Jaemin, 
Could you have a try with the patches (attachment 8505358 [details] [diff] [review] and attachment 8506600 [details] [diff] [review])?
Flags: needinfo?(jaemin1.song)
(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.
Flags: needinfo?(cpearce)
(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.
Flags: needinfo?(jaemin1.song)
(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.
Attachment #8506600 - Attachment description: Bug-1059661-Jump-to-the-previous-key-frame-and-do-ac.patch → Bug-1059661-Jump-to-the-previous-key-frame-and-do-accureate-seek-when no-next-key-frame
Attachment #8506600 - Flags: review?(sotaro.ikeda.g)
Attachment #8506600 - Flags: review?(cpearce)
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.
Attachment #8506600 - Flags: review?(cpearce) → review+
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.
Attachment #8506600 - Flags: review?(sotaro.ikeda.g)
(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!
1. Add mutex protection when changing mIsVideoSeeking as comment 62. 
2. Remove android log in MDSM added in attachment 8506600 [details] [diff] [review].
Attachment #8506600 - Attachment is obsolete: true
Attachment #8508426 - Flags: review?(sotaro.ikeda.g)
Attachment #8508426 - Flags: review?(sotaro.ikeda.g) → review+
(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.
Carry r+ from sotaro and cpearce.
Attachment #8508426 - Attachment is obsolete: true
Attachment #8509330 - Flags: review+
I don't see any b2g32 approvals on these patches?
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: checkin-needed
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
Attachment #8505350 - Flags: approval-mozilla-b2g32?
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
Attachment #8505358 - Flags: approval-mozilla-b2g32?
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
Attachment #8509330 - Flags: approval-mozilla-b2g32?
(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.
Attachment #8505350 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Attachment #8505358 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Attachment #8509330 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Keywords: checkin-needed
Depends on: 1101742
Posted video video
Depends on: 1104839
Blocks: Woodduck
You need to log in before you can comment on or make changes to this bug.