652721 - Minor robustness improvements for Ogg seeking code

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

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]]

Attachment: original patch

Comment 2

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

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]]

Attachment: rebased patch

Thanks for the r+.  Rebased patch with commit message attached.

Comment 4

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/6dca486181bd

Comment 5

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

http://hg.mozilla.org/mozilla-central/rev/6dca486181bd

Comment 6

[LegNeato]

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 (Not reading bugmail)]]

On Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/5f3522e3e4ae

Comment 9

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

On Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/10eff2860eaf

Comment 10

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

This code is not in 1.9.2.

Comment 11

[Mihaela Velimiroviciu (:mihaelav)]

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]]

There's not really a way to test this.  The best you can do is to try bug 645773 comment 0.

Comment 13

[u279076]

@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

[u279076]

qa+: fix verification needed on Firefox 7, see comment 13.

Comment 15

[Mihaela Velimiroviciu (:mihaelav)]

Tried several times to see a stream on air.mozilla but there wasn't any

Comment 16

[u279076]

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

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

[u279076]

(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

[u279076]

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)]

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)]

(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

[u279076]

Reopening for clarification if this is an unresolved issue on this bug or if it is a different bug.

Comment 23

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

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

[u279076]

(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

[u279076]

(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 (Not reading bugmail)]]

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

[u279076]

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

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

[u279076]

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 (Not reading bugmail)]]

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]]

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

[u279076]

Marking VERIFIED based on recent comments.