Closed Bug 945475 Opened 11 years ago Closed 10 years ago

Intermittent test_reset_src.html | videoHeight should be zero when there is no video. - got 300, expected 0 | videoWidth should be zero when there is no video. - got 400, expected 0

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla31
Tracking Flags:
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- fixed
firefox-esr24 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: RyanVM, Assigned: jwwang)

Details

(Keywords: intermittent-failure)

Attachments

(3 files)

part1_fix_v1.patch
2.21 KB, patch
roc
: review+
Details | Diff | Splinter Review
part1_fix_aurora.patch
2.10 KB, patch
jwwang
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
part1_fix_beta.patch
2.10 KB, patch
jwwang
: review+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review 
https://tbpl.mozilla.org/php/getParsedLog.php?id=31319325&tree=Mozilla-Inbound

Android 4.0 Panda mozilla-inbound opt test mochitest-3 on 2013-12-02 07:37:13 PST for push 04ab309c684a
slave: panda-0086

07:59:32     INFO -  18555 INFO TEST-START | /tests/content/media/test/test_reset_src.html
07:59:32     INFO -  18556 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | We should have a videoHeight.
07:59:32     INFO -  18557 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | We should have a videoWidth.
07:59:32     INFO -  18558 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | mozPaintedFrames should be positive, is 32.
07:59:32     INFO -  18559 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | mozFrameDelay should be positive or zero, is 0.011788694.
07:59:32     INFO -  18560 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | We should have a videoHeight.
07:59:32     INFO -  18561 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | We should have a videoWidth.
07:59:32     INFO -  18562 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | mozPaintedFrames should be positive, is 38.
07:59:32     INFO -  18563 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | mozFrameDelay should be positive or zero, is 0.020652729.
07:59:32     INFO -  18564 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_reset_src.html | videoHeight should be zero when there is no video. - got 300, expected 0
07:59:32     INFO -  18565 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_reset_src.html | videoWidth should be zero when there is no video. - got 400, expected 0
07:59:32     INFO -  18566 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | mozPaintedFrames should be zero when there is no video.
07:59:32     INFO -  18567 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | mozFrameDelay should be zero when there is no video.
07:59:32     INFO -  18568 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | Trying to draw to a canvas should throw, since we don't have a frame anymore
07:59:32     INFO -  18569 INFO TEST-INFO | MEMORY STAT vsize after test: 878800896
07:59:32     INFO -  18570 INFO TEST-INFO | MEMORY STAT residentFast after test: 216948736
07:59:32     INFO -  18571 INFO TEST-INFO | MEMORY STAT heapAllocated after test: 38739972
07:59:32     INFO -  18572 INFO TEST-END | /tests/content/media/test/test_reset_src.html | finished in 3690ms
Assignee: nobody → edwin
Status: NEW → ASSIGNED
Edwin, I don't suppose you were going to circle back around to this at some point?
Flags: needinfo?(edwin)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #58)
> Edwin, I don't suppose you were going to circle back around to this at some
> point?

I have quite a lot on my plate at the moment; I think jwwang is looking at media oranges at the moment, though.
Flags: needinfo?(edwin)
JW, are you able to take this?
Flags: needinfo?(jwwang)
sure.
Assignee: edwin → jwwang
Flags: needinfo?(jwwang)
root cause:
1. 'src' is changed from a video to an audio file in the test
2. HTMLMediaElement::ResetState() is called in HTMLMediaElement::Load() to reset |mMediaSize| to (-1,-1).
3. pending MediaDecoder::PlaybackPositionChanged() calls MediaDecoder::Invalidate() which indirectly calls HTMLMediaElement::UpdateMediaSize() to overwrite |mMediaSize| and results in staled videoWidth/videoHeight.

fix:
Call VideoFrameContainer::ForgetElement() to prevent staled events from reach the media element.
see comment 74 for what this patch does.
Attachment #8397557 - Flags: review?(cpearce)
Comment on attachment 8397557 [details] [diff] [review]
part1_fix_v1.patch

Review of attachment 8397557 [details] [diff] [review]:
-----------------------------------------------------------------

ImageContainer/VideoFrameContainer is roc's territory, passing review to him...
Attachment #8397557 - Flags: review?(cpearce) → review?(roc)
(In reply to JW Wang[:jwwang] from comment #74)
Sorry, I got the root cause wrong. Since |mShuttingDown| is true (HTMLMediaElement::Load => HTMLMediaElement::AbortExistingLoads => HTMLMediaElement::ShutdownDecoder => MediaDecoder::Shutdown) in MediaDecoder::PlaybackPositionChanged(), MediaDecoder::Invalidate won't be called.

The correct root cause is as follows:
1. 'src' is changed from a video to an audio file in the test
2. VideoFrameContainer::Reset => |mIntrinsicSize| = (-1,-1)
3. MediaDecoderStateMachine::RenderVideoFrame => VideoFrameContainer::SetCurrentFrame => |mIntrinsicSizeChanged| = true
4. MediaDecoder::MetadataLoaded => MediaDecoder::Invalidate => VideoFrameContainer::InvalidateWithFlags => HTMLMediaElement::UpdateMediaSize

For calling RenderVideoFrame() is wrapping in a ReentrantMonitorAutoExit monitor, it allows main thread to cut in and change the decoder state during the execution of RenderVideoFrame(). It seems to me that wherever ReentrantMonitorAutoExit is used in MediaDecoderStateMachine, we have to check the decoder state again to prevent staled changes propagated to the main thread. (Bug 760770 might be related)

Fix:
Call VideoFrameContainer::ForgetElement() to prevent staled events from reaching the media element. Also delete VideoFrameContainer more aggressively to prevent it from being shared between multiple decoders or DOMMediaStreams.
Hi Roc,
Can you help review the patch? The cause and fix are explained in Comment 81. Thanks.
Flags: needinfo?(roc)
try is green: https://tbpl.mozilla.org/?tree=Try&rev=339f38804141
Flags: needinfo?(roc)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b16e2cb162f5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Please request Aurora/Beta approval on this when you get a chance :)
status-b2g-v1.3: --- → unaffected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → fixed
status-firefox29: --- → affected
status-firefox30: --- → affected
status-firefox31: --- → fixed
status-firefox-esr24: --- → unaffected
Flags: needinfo?(jwwang)
I am on a PTO until 4/9. Will get it done once I am back. :)
Flags: needinfo?(jwwang)
The following changeset is now in Firefox Nightly:

> b16e2cb162f5 Bug 945475 - clear |mVideoFrameContainer| to stop staled callbacks which give incorrect videoWidth/videoHeight. r=roc

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central
[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: user may get incorrect videoWidth/videoHeight after changing 'src' attribute
Testing completed (on m-c, etc.): try: https://tbpl.mozilla.org/?tree=Try&rev=da1ed7aa4620
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8405132 - Flags: review+
Attachment #8405132 - Flags: approval-mozilla-aurora?
[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: user may get incorrect videoWidth/videoHeight after changing 'src' attribute
Testing completed (on m-c, etc.): try: https://tbpl.mozilla.org/?tree=Try&rev=c37658a90ee3
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8405136 - Flags: review+
Attachment #8405136 - Flags: approval-mozilla-beta?
Attachment #8405132 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8405136 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: