Bug 125270 - [MSE] Bring end-of-stream algorithm section up to current spec.
Summary: [MSE] Bring end-of-stream algorithm section up to current spec.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on: 125269
Blocks:
  Show dependency treegraph
 
Reported: 2013-12-04 17:45 PST by Jer Noble
Modified: 2013-12-09 09:01 PST (History)
8 users (show)

See Also:


Attachments
Patch (23.00 KB, patch)
2013-12-05 23:40 PST, Jer Noble
darin: review+
Details | Formatted Diff | Diff
Patch for landing (23.21 KB, patch)
2013-12-06 17:50 PST, Jer Noble
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (467.18 KB, application/zip)
2013-12-06 19:49 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (529.62 KB, application/zip)
2013-12-06 21:35 PST, Build Bot
no flags Details
Patch for landing (24.45 KB, patch)
2013-12-07 13:35 PST, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2013-12-04 17:45:04 PST
[MSE] Bring end-of-stream algorithm section up to current spec.
Comment 1 Jer Noble 2013-12-05 23:40:54 PST
Created attachment 218575 [details]
Patch
Comment 2 Darin Adler 2013-12-06 10:21:30 PST
Comment on attachment 218575 [details]
Patch

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

This looks good. The patch did not apply, so EWS did not process it. I can’t tell how well the test covers all the code paths.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:344
> +    if (error.isNull() || error.isEmpty()) {

Null strings return true for isEmpty so there is no need to check both. Can just check isEmpty.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:368
> +        if (m_mediaElement->readyState() == HTMLMediaElement::HAVE_NOTHING) {
> +            //  âªï¸ If the HTMLMediaElement.readyState attribute equals HAVE_NOTHING
> +            //    Run the "If the media data cannot be fetched at all, due to network errors, causing
> +            //    the user agent to give up trying to fetch the resource" steps of the resource fetch algorithm.
> +            m_mediaElement->mediaLoadingFailed(MediaPlayer::NetworkError);
> +        } else {
> +            //  âªï¸ If the HTMLMediaElement.readyState attribute is greater than HAVE_NOTHING
> +            //    Run the "If the connection is interrupted after some media data has been received, causing the
> +            //    user agent to give up trying to fetch the resource" steps of the resource fetch algorithm.
> +            m_mediaElement->mediaLoadingFailedFatally(MediaPlayer::NetworkError);
> +        }

The comments here do not make it clear why one case calls “failed fatally” and the other does not. I guess the problem is that literally quoting this piece of the spec ends up meaning we don’t say enough.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:384
> +        return;

Seems unnecessary to have a return just before the end of the function.

> Source/WebCore/Modules/mediasource/MediaSource.h:83
>      void endOfStream(const AtomicString& error, ExceptionCode&);
> +    void endOfStreamAlgorithm(const AtomicString& error, ExceptionCode&);

I don’t think the names here are clear enough. It’s not like “end of stream” is not already an algorithm. I know there is some term of art or concept in the spec we are referring to here, but I would suggest making the names more distinct, probably also including a verb in the name of the new function. And also I would suggest not paragraphing this internal function right next to public functions from the DOM API.
Comment 3 Jer Noble 2013-12-06 10:47:09 PST
(In reply to comment #2)
> (From update of attachment 218575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218575&action=review
> 
> This looks good. The patch did not apply, so EWS did not process it. I can’t tell how well the test covers all the code paths.

Yes, this patch depends on a patch which hasn't been committed yet.  I'll upload a "Patch for committing" and let the EWS chew on it before landing.

> > Source/WebCore/Modules/mediasource/MediaSource.cpp:344
> > +    if (error.isNull() || error.isEmpty()) {
> 
> Null strings return true for isEmpty so there is no need to check both. Can just check isEmpty.

Will do.

> > Source/WebCore/Modules/mediasource/MediaSource.cpp:368
> > +        if (m_mediaElement->readyState() == HTMLMediaElement::HAVE_NOTHING) {
> > +            //  âªï¸ If the HTMLMediaElement.readyState attribute equals HAVE_NOTHING
> > +            //    Run the "If the media data cannot be fetched at all, due to network errors, causing
> > +            //    the user agent to give up trying to fetch the resource" steps of the resource fetch algorithm.
> > +            m_mediaElement->mediaLoadingFailed(MediaPlayer::NetworkError);
> > +        } else {
> > +            //  âªï¸ If the HTMLMediaElement.readyState attribute is greater than HAVE_NOTHING
> > +            //    Run the "If the connection is interrupted after some media data has been received, causing the
> > +            //    user agent to give up trying to fetch the resource" steps of the resource fetch algorithm.
> > +            m_mediaElement->mediaLoadingFailedFatally(MediaPlayer::NetworkError);
> > +        }
> 
> The comments here do not make it clear why one case calls “failed fatally” and the other does not. I guess the problem is that literally quoting this piece of the spec ends up meaning we don’t say enough.

Yeah, unfortunately, the MSE spec just refers to the HTML Media spec, which explicitly says that loading should be a soft failure vs. a hard one.

> > Source/WebCore/Modules/mediasource/MediaSource.cpp:384
> > +        return;
> 
> Seems unnecessary to have a return just before the end of the function.
> 
> > Source/WebCore/Modules/mediasource/MediaSource.h:83
> >      void endOfStream(const AtomicString& error, ExceptionCode&);
> > +    void endOfStreamAlgorithm(const AtomicString& error, ExceptionCode&);
> 
> I don’t think the names here are clear enough. It’s not like “end of stream” is not already an algorithm. I know there is some term of art or concept in the spec we are referring to here, but I would suggest making the names more distinct, probably also including a verb in the name of the new function. And also I would suggest not paragraphing this internal function right next to public functions from the DOM API.

Okay, I'm not a huge fan of the name either.  I'll rename this to streamEndedWithError().
Comment 4 Jer Noble 2013-12-06 17:50:00 PST
Created attachment 218639 [details]
Patch for landing

Darin, I added an extra note at each of those confusing call sites, but I'm not certain they do much more te explain the following code than the code does itself.
Comment 5 Build Bot 2013-12-06 19:49:41 PST
Comment on attachment 218639 [details]
Patch for landing

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

New failing tests:
media/media-source/media-source-fastseek.html
media/media-source/media-source-play.html
Comment 6 Build Bot 2013-12-06 19:49:43 PST
Created attachment 218644 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 7 Build Bot 2013-12-06 21:35:51 PST
Comment on attachment 218639 [details]
Patch for landing

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

New failing tests:
media/media-source/media-source-fastseek.html
media/media-source/media-source-play.html
Comment 8 Build Bot 2013-12-06 21:35:54 PST
Created attachment 218649 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 9 Jer Noble 2013-12-07 13:35:50 PST
Created attachment 218666 [details]
Patch for landing
Comment 10 Jer Noble 2013-12-07 16:42:37 PST
Marking this bug as depending on bug #125269, which fixes the crashes seen on some of the EWS bots.  I'll wait till that one is reviewed and lands before committing this one.
Comment 11 Jer Noble 2013-12-07 22:40:49 PST
Committed r160282: <http://trac.webkit.org/changeset/160282>
Comment 12 Simon Fraser (smfr) 2013-12-08 10:09:11 PST
Could this have caused two tests to show malloc errors?
http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r160286%20(853)/results.html
Comment 13 Jer Noble 2013-12-09 08:20:21 PST
(In reply to comment #12)
> Could this have caused two tests to show malloc errors?
> http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r160286%20(853)/results.html

It's unlikely; those test cases should never hit MSE code.  But i'll check.
Comment 14 Jer Noble 2013-12-09 08:45:39 PST
(In reply to comment #13)
> (In reply to comment #12)
> > Could this have caused two tests to show malloc errors?
> > http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r160286%20(853)/results.html
> 
> It's unlikely; those test cases should never hit MSE code.  But i'll check.

I haven't updated my local tree since committing this change, and running those tests with MallocScribble enabled doesn't cause a crash (as you might expect if some new code was modifying freed objects). But since there's nothing suspicious between r160282 (this change) and r160286, it's either an old bug or these patches caused it.
Comment 15 Jer Noble 2013-12-09 09:01:59 PST
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Could this have caused two tests to show malloc errors?
> > > http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r160286%20(853)/results.html
> > 
> > It's unlikely; those test cases should never hit MSE code.  But i'll check.
> 
> I haven't updated my local tree since committing this change, and running those tests with MallocScribble enabled doesn't cause a crash (as you might expect if some new code was modifying freed objects). But since there's nothing suspicious between r160282 (this change) and r160286, it's either an old bug or these patches caused it.

Okay, probably not this patch, as it's been happening for a while: http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r160276%20(849)/results.html

It's crashing in a different track test each time, but always in track/.  Is there already a bug for this issue, or should I file a new one?