New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Oct 2011
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment
link

Issue 98955: Renderer hangs during benchmarking on http://ascherkus.github.com/iheartvideo/benchmark.html

Reported by scherkus@chromium.org, Oct 3 2011 Project Member

Issue description

Keep running http://ascherkus.github.com/iheartvideo/benchmark.html until the tab hangs.

Seems to happen on Windows but I think I've seen it happen on Linux as well.
 

Comment 1 by scherkus@chromium.org, Oct 4 2011

Labels: -OS-Windows OS-Linux
Happening on Linux as well.

Comment 2 by scherkus@chromium.org, Oct 4 2011

Labels: -OS-Linux OS-All

Comment 3 by vrk@chromium.org, Oct 6 2011

Owner: vrk@chromium.org
Hang seems to happen consistently on http://vimeo.com/6540668 (running linux chrome 16.0.902.0 (Developer Build 104109)).

Comment 4 by fischman@google.com, Oct 6 2011

Owner: fischman@chromium.org

Comment 5 by bugdroid1@chromium.org, Oct 7 2011

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=104540

------------------------------------------------------------------------
r104540 | fischman@chromium.org | Fri Oct 07 12:12:21 PDT 2011

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/media/base/composite_filter.cc?r1=104540&r2=104539&pathrev=104540
 M http://src.chromium.org/viewvc/chrome/trunk/src/media/filters/ffmpeg_video_decoder.cc?r1=104540&r2=104539&pathrev=104540
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/media/audio_renderer_impl.cc?r1=104540&r2=104539&pathrev=104540
 M http://src.chromium.org/viewvc/chrome/trunk/src/media/filters/ffmpeg_demuxer.cc?r1=104540&r2=104539&pathrev=104540
 M http://src.chromium.org/viewvc/chrome/trunk/src/media/filters/video_renderer_base.cc?r1=104540&r2=104539&pathrev=104540

Fix hangs & crashes in teardown-during-Seek.

Hangs:
- AudioRendererImpl: Don't hold lock_ in the call to Join() b/c OnStop wants to
  acquire the lock before exiting.
- FFmpegDemuxerStream::Stop: signal EOS to waiting read callbacks (instead of
  silently dropping them on the floor) so decoders can exit instead of stalling
  (and causing renderers to stall).

Crashes:
- CompositeFilter::Stop: clear out the Seek callback when Stopping.
- FFmpegVideoDecoder::ProduceVideoFrame: early-return if already stopped (racing
  with webkit repaints makes this necessary).

Also fixed a bunch of mis-indentation where I saw it.

BUG= 98975 , 98948 , 98955 , 96861 
TEST=trybots, manual


Review URL: http://codereview.chromium.org/8184003
------------------------------------------------------------------------

Comment 6 by fischman@google.com, Oct 7 2011

Labels: -Mstone-15 -ReleaseBlock-Stable Mstone-16
This is fixed in trunk.  Decided w/ scherkus@ to not merge into m15 because:
1) It's unclear what the impact is on mainstream use-cases
2) A merge would be tricky (difficult to ensure correct) b/c of interleaved refactorings (Bind & low-latency audio codepaths, specifically).

If this turns out to be a significant problem in the wild we might reconsider this decision.

Comment 7 by fischman@google.com, Oct 7 2011

Status: Fixed

Comment 8 by vrk@chromium.org, Oct 17 2011

Labels: -Mstone-16 Mstone-15 Merge-Requested
Looks like YT does change the src of the video... @fischman how risky would it be to merge?

Comment 9 by fischman@google.com, Oct 17 2011

vrk: low-risk IMO, except it assumes low-latency codepath, which was reverted for m15.  
(so the DCHECK at http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/media/audio_renderer_impl.cc?annotate=104540&pathrev=104540#l468 shouldn't be merged)

Comment 10 by fischman@google.com, Oct 17 2011

Status: Available
Re-opening to avoid being ignored for merge request.

Comment 11 by kareng@google.com, Oct 17 2011

looks like we need a specific slightly-different CL for 874?

Comment 12 by fischman@google.com, Oct 17 2011

kareng: possibly; I'm currently looking at the results of attempting a drover+edit of the original patch.

Comment 13 by fischman@google.com, Oct 17 2011

Status: Started
This is the result of manually resolving the drover-generated merge, plus the removal of the one DCHECK I described above: http://codereview.chromium.org/8320011

I have high confidence that if it builds it'll run correctly; my only small worry is that between the m15 branch and the original CL submission the base::Bind migration (new-style callbacks) happened throughout src/media/ and I might have missed some interaction that'll break compilation.

Comment 14 by fischman@google.com, Oct 17 2011

Checked out an 874 client & confirmed this patch builds correctly and fixes the hang problem on benchmark.html.

i can haz merge?

Comment 15 by kareng@google.com, Oct 17 2011

Labels: -Merge-Requested Merge-Approved
yes you can :)

Comment 16 by bugdroid1@chromium.org, Oct 17 2011

Project Member
Labels: -merge-approved merge-merged-874
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=105930

------------------------------------------------------------------------
r105930 | fischman@chromium.org | Mon Oct 17 14:47:52 PDT 2011

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/874/src/media/base/composite_filter.cc?r1=105930&r2=105929&pathrev=105930
 M http://src.chromium.org/viewvc/chrome/branches/874/src/media/filters/ffmpeg_video_decoder.cc?r1=105930&r2=105929&pathrev=105930
 M http://src.chromium.org/viewvc/chrome/branches/874/src/content/renderer/media/audio_renderer_impl.cc?r1=105930&r2=105929&pathrev=105930
 M http://src.chromium.org/viewvc/chrome/branches/874/src/media/filters/ffmpeg_demuxer.cc?r1=105930&r2=105929&pathrev=105930
 M http://src.chromium.org/viewvc/chrome/branches/874/src/media/filters/video_renderer_base.cc?r1=105930&r2=105929&pathrev=105930

Merge 104540 - Fix hangs & crashes in teardown-during-Seek.

Hangs:
- AudioRendererImpl: Don't hold lock_ in the call to Join() b/c OnStop wants to
acquire the lock before exiting.
- FFmpegDemuxerStream::Stop: signal EOS to waiting read callbacks (instead of
silently dropping them on the floor) so decoders can exit instead of stalling
(and causing renderers to stall).

Crashes:
- CompositeFilter::Stop: clear out the Seek callback when Stopping.
- FFmpegVideoDecoder::ProduceVideoFrame: early-return if already stopped (racing
with webkit repaints makes this necessary).

Also fixed a bunch of mis-indentation where I saw it.

BUG= 98975 ,  98948 ,  98955 ,  96861 
TEST=trybots, manual
Review URL: http://codereview.chromium.org/8294027
------------------------------------------------------------------------

Comment 17 by fischman@google.com, Oct 17 2011

Status: Fixed

Comment 18 by scherkus@chromium.org, Oct 19 2011

Labels: -ImportantForVideo Hotlist-NeededForVideo

Comment 19 by scherkus@chromium.org, Nov 17 2011

Cc: krisr@chromium.org rohi...@chromium.org scherkus@chromium.org anan...@chromium.org
 Issue 69343  has been merged into this issue.

Comment 20 by bugdroid1@chromium.org, Oct 13 2012

Project Member
Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.

Comment 21 by bugdroid1@chromium.org, Mar 10 2013

Project Member
Labels: -Area-WebKit -Feature-Media-Network -Mstone-15 Cr-Content Cr-Internals-Media-Network M-15

Comment 22 by bugdroid1@chromium.org, Mar 13 2013

Project Member
Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue

Comment 23 by bugdroid1@chromium.org, Apr 6 2013

Project Member
Labels: -Cr-Content Cr-Blink

Sign in to add a comment