Last Comment Bug 652721 - Minor robustness improvements for Ogg seeking code
: Minor robustness improvements for Ogg seeking code
Status: VERIFIED FIXED
[qa!]
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Matthew Gregan [:kinetik]
:
Mentors:
Depends on:
Blocks: CVE-2011-2992
  Show dependency treegraph
 
Reported: 2011-04-25 17:11 PDT by Matthew Gregan [:kinetik]
Modified: 2011-10-04 11:16 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
unaffected


Attachments
original patch (4.54 KB, patch)
2011-04-25 17:12 PDT, Matthew Gregan [:kinetik]
cpearce: review+
Details | Diff | Splinter Review
rebased patch (3.72 KB, patch)
2011-07-21 21:55 PDT, Matthew Gregan [:kinetik]
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Matthew Gregan [:kinetik] 2011-04-25 17:11:30 PDT
While doing the initial investigation into bug 645773, I made some minor robustness improvements to the Ogg seeking code.  This bug is to track getting those improvements landed.
Comment 1 Matthew Gregan [:kinetik] 2011-04-25 17:12:57 PDT
Created attachment 528232 [details] [diff] [review]
original patch
Comment 2 Chris Pearce (:cpearce) 2011-07-21 17:12:38 PDT
Comment on attachment 528232 [details] [diff] [review]
original patch

Had you meant to request review on this? Looks good to me.

>     // Ensure the offsets are after the header pages.
>-    PRInt64 startOffset = NS_MAX(cached[index].mStart, mDataOffset);
>-    PRInt64 endOffset = NS_MAX(cached[index].mEnd, mDataOffset);
>+    PRInt64 startOffset = NS_MAX(range.mStart, mDataOffset);
>+    PRInt64 endOffset = NS_MAX(range.mEnd, mDataOffset);

Bitrot here, looks like this change has already landed.

>@@ -1100,17 +1100,20 @@ nsOggReader::SelectSeekRange(const nsTAr
>-  return aExact ? SeekRange() : SeekRange(so, eo, st, et);
>+  if (aExact || eo == -1) {
>+    return SeekRange();
>+  }
>+  return SeekRange(so, eo, st, et);

We don't handle the case where |eo == -1| properly, but we won't get into this situation once bug 462960 lands. This change at least this ensures our seek range is length 0 if our media is unbounded, so lets go with this.
Comment 3 Matthew Gregan [:kinetik] 2011-07-21 21:55:40 PDT
Created attachment 547606 [details] [diff] [review]
rebased patch

Thanks for the r+.  Rebased patch with commit message attached.
Comment 5 Chris Pearce (:cpearce) 2011-07-25 10:42:39 PDT
http://hg.mozilla.org/mozilla-central/rev/6dca486181bd
Comment 6 christian 2011-07-25 15:20:20 PDT
Comment on attachment 547606 [details] [diff] [review]
rebased patch

This looks safe, approved for releases/mozilla-aurora and releases/mozilla-beta
Comment 8 Chris Pearce (:cpearce) 2011-07-25 18:52:03 PDT
On Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/5f3522e3e4ae
Comment 9 Chris Pearce (:cpearce) 2011-07-25 19:15:01 PDT
On Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/10eff2860eaf
Comment 10 Chris Pearce (:cpearce) 2011-08-08 15:08:51 PDT
This code is not in 1.9.2.
Comment 11 Mihaela Velimiroviciu (:mihaelav) 2011-08-19 05:36:59 PDT
Can anyone please provide a test case or STR / guidelines that I can use to verify this? Do I need to create any specific environment to test it?

Thanks!
Comment 12 Matthew Gregan [:kinetik] 2011-08-21 15:28:24 PDT
There's not really a way to test this.  The best you can do is to try bug 645773 comment 0.
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-08-24 11:48:58 PDT
@Mihaela if you can watch an air.mozilla stream in fullscreen without crashing on Firefox 6 and 7 you can mark this bug as VERIFIED. Thanks
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 13:34:20 PDT
qa+: fix verification needed on Firefox 7, see comment 13.
Comment 15 Mihaela Velimiroviciu (:mihaelav) 2011-09-23 06:39:46 PDT
Tried several times to see a stream on air.mozilla but there wasn't any
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-23 10:01:24 PDT
(In reply to Mihaela Velimiroviciu [QA] from comment #15)
> Tried several times to see a stream on air.mozilla but there wasn't any

Yeah, there isn't constant streaming. There will be another stream available at 11am PDT on Monday during the project-wide meeting. Please verify then.
Comment 17 Mihaela Velimiroviciu (:mihaelav) 2011-09-26 07:13:58 PDT
Isn't there any other stream available earlier in the day (or some other day) that I can check (11am PDT it's a little late in the evening in our timezone to check at work and I don't have Linux at home)? Or can someone from US check this, please?

Thank you!
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-26 09:35:11 PDT
(In reply to Mihaela Velimiroviciu [QA] from comment #17)
> Isn't there any other stream available earlier in the day (or some other
> day) that I can check (11am PDT it's a little late in the evening in our
> timezone to check at work and I don't have Linux at home)? Or can someone
> from US check this, please?
> 
> Thank you!

I'll check this today during the Mozilla Weekly Project Update stream.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-26 11:12:40 PDT
Not sure if this is a bustage in the stream or not but I can't seem to get Fullscreen mode to work properly:

1) Go to air.mozilla.com while a stream is playing
2) Right click the video and select FULLSCREEN

Result:
Video appears to be buffering but never reaches a point where it plays.
If I move the mouse it just displays the loading throbber overlay on the video

Tested with Firefox 7.0RC on Windows 7, Mac OSX 10.6.8, and Ubuntu 11.04 64-bit.
Comment 20 Mihaela Velimiroviciu (:mihaelav) 2011-09-26 11:28:45 PDT
Same as in comment 19 on Windows 7/64bit, both with Firefox 7.0RC and 6.0.2.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0.2) Gecko/20100101 Firefox/6.0.2
Comment 21 Maniac Vlad Florin (:vladmaniac) 2011-09-26 12:14:31 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #19)
> Not sure if this is a bustage in the stream or not but I can't seem to get
> Fullscreen mode to work properly:
> 
> 1) Go to air.mozilla.com while a stream is playing
> 2) Right click the video and select FULLSCREEN
> 
> Result:
> Video appears to be buffering but never reaches a point where it plays.
> If I move the mouse it just displays the loading throbber overlay on the
> video
> 
> Tested with Firefox 7.0RC on Windows 7, Mac OSX 10.6.8, and Ubuntu 11.04
> 64-bit.

Same result with testcase link cubicvr.org/CubicVR.js/bd3/track3.php 
The video there behaves like a stream, and when in fullscreen mode it just stops playing. The buffering animation is present. 
The video restarts playing from when it was left, but only when returning to normal mode.
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-26 12:17:42 PDT
Reopening for clarification if this is an unresolved issue on this bug or if it is a different bug.
Comment 23 Chris Pearce (:cpearce) 2011-09-26 14:48:51 PDT
That sounds like a new bug, can you a new bug for that please? It may be related to bug 689106 however. Thanks.
Comment 24 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-26 16:11:28 PDT
(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #23)
> That sounds like a new bug, can you a new bug for that please? It may be
> related to bug 689106 however. Thanks.

Given that, is it safe to mark this verified?
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-26 16:17:28 PDT
(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #23)
> That sounds like a new bug, can you a new bug for that please? It may be
> related to bug 689106 however. Thanks.

Filed bug 689360.
Comment 26 Chris Pearce (:cpearce) 2011-09-26 16:31:32 PDT
Thanks Anthony. The simplest way to test this would have been to try seeking into unbuffered regions in live streams, but recent UI changes and the @seekable attribute implementation cover this case as well. So you could write a simple Javascript test case that seeks into a region which is not in the @seekable time ranges on a live stream, and if that doesn't crash, you could mark this one as verified. That would also enable you to mark bug 462960 as verified as well.

You may find my media test server handy as that can imitate live streams and allows you to control the rate at which media served:
http://blog.pearce.org.nz/2011/08/simple-rate-limited-http-server-for.html
Ping me on IRC if you need some pointers on getting this working.
Comment 27 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-26 16:36:15 PDT
(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #26)
> Ping me on IRC if you need some pointers on getting this working.

Given the timeline to release is less than 24 hours I don't think it is realistic for me to figure out how to code a testcase from scratch. If you have time to help by creating the minimized testcase, it would be greatly appreciated.

Thanks
Comment 28 Vlad [QA] 2011-10-03 07:35:49 PDT
I have used the steps from comment21 in order to verify this bug. When in full screen mode, the stream doesn't stop anymore but it's start from the beginning and when exiting full screen mode, sometimes the video freezes.
Is this an issue?
Can I change the status of this bug?

Verified on:

Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111002 Firefox/9.0a2
Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111002 Firefox/10.0a1

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a2) Gecko/20111002 Firefox/9.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a1) Gecko/20110929 Firefox/10.0a1

Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Windows NT 5.1; rv:9.0a2) Gecko/20111002 Firefox/9.0a2
Mozilla/5.0 (Windows NT 5.1; rv:10.0a1) Gecko/20111002 Firefox/10.0a1


Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a2) Gecko/20111002 Firefox/9.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20110930 Firefox/10.0a1
Comment 29 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-03 10:37:04 PDT
Chris Pearce, do you call this VERIFIED based on comment 28? or do we really need to code a JS testcase?

Vlad, this bug does not cover video freezing during fullscreen mode.
Comment 30 Chris Pearce (:cpearce) 2011-10-03 14:17:52 PDT
It's not clear what Vlad was verifying in comment 28. This bug has nothing to do with full-screen mode.

I outlined steps in comment 26 on how to verify this bug. You can also follow the STR in bug 672789 comment 1.
Comment 31 Vlad [QA] 2011-10-04 00:23:27 PDT
Like I said in comment28, when in full screen mode, the video keeps on playing. So imo this is verified fixed.
I verified this by following the steps from comment21 (Anthony and Vlad's str)

Regarding the full screen issue, I'll post in bug 689360.
Comment 32 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-04 11:16:04 PDT
Marking VERIFIED based on recent comments.

Note You need to log in before you can comment on or make changes to this bug.