Last Comment Bug 768333 - nsBufferedAudioStream::GetPositionInFrames reports bogus values in nightly Win32 builds
: nsBufferedAudioStream::GetPositionInFrames reports bogus values in nightly Wi...
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla17
Assigned To: Matthew Gregan [:kinetik]
:
Mentors:
http://flim.org/~kinetik/gbc/mozaudio...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-25 22:44 PDT by Matthew Gregan [:kinetik]
Modified: 2012-08-10 05:38 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified


Attachments
patch v0 (2.20 KB, patch)
2012-07-16 14:21 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Review
patch v1 (1.97 KB, patch)
2012-07-30 18:59 PDT, Matthew Gregan [:kinetik]
cpearce: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review
followup v0 (733 bytes, patch)
2012-07-31 02:13 PDT, Matthew Gregan [:kinetik]
cpearce: review+
Details | Diff | Review

Description Matthew Gregan [:kinetik] 2012-06-25 22:44:22 PDT
Open the linked testcase, click "4410", note that p is some insanely large value instead of something between 0 and 4410.

I can reproduce this in official nightly builds, but not my own optimized or debug builds (with VC++ 2008 or 2010).  Presumably the major difference between my VC++ 2010 optimized builds the nightlies is that mine lack PGO.

It looks like the problem is inside GetPositionInFrames():

  return GetPositionInFramesUnlocked();
016AC6C5  push        esi  
016AC6C6  call        nsBufferedAudioStream::GetPositionInFramesUnlocked (16AC638h) 
016AC6CB  push        eax  
016AC6CC  push        edx  
016AC6CD  push        dword ptr [esi+18h] 
016AC6D0  call        dword ptr [__imp__PR_Unlock (1B93320h)] 
016AC6D6  pop         ecx  
016AC6D7  pop         eax  
016AC6D8  pop         edx  
016AC6D9  pop         esi  
}
016AC6DA  ret              

Note that eax and edx are saved to the stack before calling PR_Unlock, but they're restored in the wrong order, resulting in the high and low part of the result being exchanged.
Comment 1 Matthew Gregan [:kinetik] 2012-06-25 23:12:53 PDT
Disassembled versions of GetPositionInFramesUnlocked() and the caller (nsHTMLAudioElement::MozCurrentSampleOffset()) in local optimized and nightly PGO builds are more or less identical, but my local optimized build has quite a different GetPositionInFrames():

  return GetPositionInFramesUnlocked();
0150BB0B  mov         ecx,esi 
0150BB0D  call        nsBufferedAudioStream::GetPositionInFramesUnlocked (150B7EAh) 
0150BB12  mov         esi,eax 
0150BB14  mov         eax,dword ptr [mon] 
0150BB17  push        dword ptr [eax] 
0150BB19  mov         edi,edx 
0150BB1B  call        dword ptr [__imp__PR_Unlock (18D8214h)] 
0150BB21  pop         ecx  
0150BB22  mov         edx,edi 
0150BB24  pop         edi  
0150BB25  mov         eax,esi 
0150BB27  pop         esi  
}

Note that it saves eax and edx into esi and edi rather than using the stack, and restores them correctly after PR_Unlock.
Comment 2 Matthew Gregan [:kinetik] 2012-07-16 12:34:48 PDT
Tightened up some type handling, let's see if this helps: https://tbpl.mozilla.org/?tree=Try&rev=b382e3f5c627
Comment 3 Matthew Gregan [:kinetik] 2012-07-16 14:21:44 PDT
Created attachment 642726 [details] [diff] [review]
patch v0

This is the patch I pushed.  I'll check the resulting binary from the try build before requesting review.
Comment 4 Matthew Gregan [:kinetik] 2012-07-17 10:35:18 PDT
Comment on attachment 642726 [details] [diff] [review]
patch v0

This patch doesn't work.
Comment 5 Matthew Gregan [:kinetik] 2012-07-29 23:24:03 PDT
Let's try disabling PGO for that function: https://tbpl.mozilla.org/?tree=Try&rev=04115470bdc1
Comment 6 Matthew Gregan [:kinetik] 2012-07-30 18:59:27 PDT
Created attachment 647401 [details] [diff] [review]
patch v1

Disabling optimization of that functions does the trick.  Kind of gross, but I don't have a better fix on hand.
Comment 7 Matthew Gregan [:kinetik] 2012-07-30 20:30:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/376aa1851c9a

Now that there's a fix in hand, we'll want this on the other branches that cubeb (bug 623444) landed on.
Comment 8 Matthew Gregan [:kinetik] 2012-07-30 20:30:56 PDT
Comment on attachment 647401 [details] [diff] [review]
patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 623444
User impact if declined: mozCurrentSampleOffset broken in Win32 PGO builds
Testing completed (on m-c, etc.): completed locally
Risk to taking this patch (and alternatives if risky): very low, disables optimization for a simple getter function
String or UUID changes made by this patch:
Comment 9 neil@parkwaycc.co.uk 2012-07-31 01:24:13 PDT
Comment on attachment 647401 [details] [diff] [review]
patch v1

>+  PRInt64 position = mAudioStream->GetPositionInFrames();
>+  if (position < 0) {
>+    *aRetVal = 0;
>+  } else {
>+    *aRetVal = mAudioStream->GetPositionInFrames() * mChannels;
[Why not position * mChannels?]
Comment 10 Matthew Gregan [:kinetik] 2012-07-31 02:13:06 PDT
Created attachment 647470 [details] [diff] [review]
followup v0

(In reply to neil@parkwaycc.co.uk from comment #9)
> [Why not position * mChannels?]

It should be, I just missed replacing the second call.  Thanks!
Comment 11 Ed Morley [:emorley] 2012-07-31 06:09:20 PDT
https://hg.mozilla.org/mozilla-central/rev/376aa1851c9a
Comment 12 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-31 14:40:55 PDT
Comment on attachment 647401 [details] [diff] [review]
patch v1

low risk, approving.
Comment 13 Matthew Gregan [:kinetik] 2012-07-31 18:10:02 PDT
Landed the followup patch: http://hg.mozilla.org/integration/mozilla-inbound/rev/ae767e8de495

I'll roll that tiny change into the first patch when landing on aurora and beta.
Comment 15 Ed Morley [:emorley] 2012-08-01 02:52:12 PDT
https://hg.mozilla.org/mozilla-central/rev/ae767e8de495
Comment 16 Paul Silaghi, QA [:pauly] 2012-08-02 07:58:28 PDT
(In reply to Matthew Gregan [:kinetik] from comment #0)
> Open the linked testcase, click "4410", note that p is some insanely large
> value instead of something between 0 and 4410.

I might not understand the issue. First time I click "4410" or any other buttons, p=0. Second time I click "4410", p=5364414152704.
Tested on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0b3

Isn't this landed into FF 15b3 ?
I got different results on Nightly 17.0a1 (2012-08-02), p=812 when clicked twice on "4410".
Comment 17 Matthew Gregan [:kinetik] 2012-08-02 16:16:05 PDT
(In reply to Paul Silaghi [QA] from comment #16)
> I might not understand the issue. First time I click "4410" or any other
> buttons, p=0. Second time I click "4410", p=5364414152704.
> Tested on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101
> Firefox/15.0b3

That's the broken behaviour, any time p is larger than rw we're hitting this bug.

> I got different results on Nightly 17.0a1 (2012-08-02), p=812 when clicked
> twice on "4410".

That's the correct behaviour.

> Isn't this landed into FF 15b3 ?

It looks like it missed the 15b3 build (it was tagged at Tue Jul 31 14:54:57 2012 -0700 and I landed at Tue Jul 31 18:30:51 2012 -0700).

I've just confirmed the patch works as expected in the latest Aurora build.  I guess we'll need to wait for 15b4 to confirm it in 15.
Comment 18 Paul Silaghi, QA [:pauly] 2012-08-10 05:37:26 PDT
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0b4
Comment 19 Paul Silaghi, QA [:pauly] 2012-08-10 05:38:24 PDT
Verified fixed on FF Aurora 16 based on comment 17.

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