Bug 133459

Summary: REGRESSION(169440): Repro crash when playing a video with captions
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: MediaAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, calvaris, commit-queue, eric.carlson, esprehn+autocc, gyuyoung.kim, philipj, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
eric.carlson: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 none

Description Brent Fulgham 2014-06-02 13:59:05 PDT
My recent change <http://trac.webkit.org/changeset/169440> introduced a crash when viewing video with paint-on captions. The problems were related to two issues:

1. WebKit was properly identifying new cues that extended existing cues, but was not updating the bookkeeping so that the extended cue would be updated to reflect the proper end time of the cue. This resulted in some of these cues sticking around for the remaining duration of the video.
2. The crash was being caused by the logic that updates the cue time. It removes and then re-adds the cue during the time modification, but was not updating the reference count on the cue. This caused the cue to be cleaned up during the update operation, causing the crash.

This change corrects both of these problems.
Comment 1 Brent Fulgham 2014-06-02 13:59:13 PDT
<rdar://problem/17079081>
Comment 2 Brent Fulgham 2014-06-02 14:03:33 PDT
Created attachment 232398 [details]
Patch
Comment 3 Brent Fulgham 2014-06-05 17:39:29 PDT
Created attachment 232595 [details]
Patch
Comment 4 Eric Carlson 2014-06-05 21:43:42 PDT
Comment on attachment 232595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232595&action=review

> Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:405
> +                    LOG(Media, "InbandTextTrackPrivateAVF::processCue(%p) - found an extension cue for time = %.2f, position =  %.2f, line =  %.2f", this, arrivingCue->startTime(), arrivingCue->position(), arrivingCue->line());

Won't this log about every cue regardless of whether or not it extends a cue?
Comment 5 Build Bot 2014-06-05 22:46:54 PDT
Comment on attachment 232595 [details]
Patch

Attachment 232595 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6501615309684736

New failing tests:
media/W3C/audio/canPlayType/canPlayType_application_octet_stream_with_codecs_1.html
Comment 6 Build Bot 2014-06-05 22:46:58 PDT
Created attachment 232604 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 7 Brent Fulgham 2014-06-06 08:55:05 PDT
Comment on attachment 232595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232595&action=review

>> Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:405
>> +                    LOG(Media, "InbandTextTrackPrivateAVF::processCue(%p) - found an extension cue for time = %.2f, position =  %.2f, line =  %.2f", this, arrivingCue->startTime(), arrivingCue->position(), arrivingCue->line());
> 
> Won't this log about every cue regardless of whether or not it extends a cue?

Whoops! Yes, I'll fix that.
Comment 8 Brent Fulgham 2014-06-06 09:21:06 PDT
Committed r169647: <http://trac.webkit.org/changeset/169647>