822933 - WebM playback regression

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

The linked WebM video does not play in currently nightlies.  Immediately after loading it jumps to the end of the media, and attempts to seek or replay fail.  It looks like it's failing to load, as the media element is not displayed at the correct size.

Quick regression hunt:
Firefox 17 works
Firefox 19 nightly (2012-11-19-03-07-25) works
Firefox 20 nightly (2012-12-10-03-07-47) broken
Current nightly broken

Comment 1

[Matthew Gregan [:kinetik]]

I got pretty lucky with my quick search, a complete bisection reveals 2012-12-10-03-07-47 is the first broken nightly.  That gives a changeset range on m-c of 4e83d0987a31:725eb8792d27.  There are only two changes to content/media in that range, and bug 816949 looks like the culprit.

The linked file claims to have cues:
| + Seek entry at 90
|  + Seek ID: 0x1c 0x53 0xbb 0x6b (KaxCues) at 93
|  + Seek position: 39834097 at 100

...but the file is only 39256064 octets long, placing the cues 578033 beyond the end of the file.

+  haveCues = nestegg_get_cue_point(mContext, 0, -1, &dummy, &dummy) == 0;

This isn't a valid way to test for cues.  An error returned by this means an error occurred, not that there are no cues.  In this case, what's happening is that ne_io_seek is failing to seek 578033 bytes past the EOF and failing, propagating the error back up.  When nestegg_get_cue_point returns an error, the reader should call Cleanup() and return an error (as it does in the other place nestegg_get_cue_point is used).

Comment 2

[Matthew Gregan [:kinetik]]

It also introduces a seek during the initial load to fetch the cues (if they're at the end of the file, which they generally are), which is something we had previously deferred to the first seek to help with initial load times.

Comment 3

[Paul Adenot (:padenot)]

So, what would be a valid way to check whether the media is seekable (i.e. has cues, in the current state of our implementation), that would not provoke a range request, using nestegg?

I checked libnestegg for an API call that could give me this info, but had no luck, and somehow, the |nestegg_get_cue_point| function seemed to do the job, but I obviously overlooked the implications of using it, sorry about that.

Comment 4

[Paul Adenot (:padenot)]

Attachment: Properly check if a WebM file has cues. r=

So, I've stopped being lazy and checked what libnestegg does under the hood.

This patch add an public function to check if cues are present in a file, and
relies on what the seek head tell us, hence, does not seek to check if the file
is seekable. It works as intended, afaik, and we can play the video you linked.

However, this does not handle the fact of a seek head referencing an offset that
is past the end of the file, of course, since libnestegg is not aware of the
transport layer. If the server is sending a Content-Length, we could do
something, but the server can also lie when it sends the Content-Length back, so
we cannot be 100% sure anyways.

I'll be making test file for that, so we won't regress in the future. I plan to
do a webm file that has no cues, and a webm file that has a bogus offset.

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 695741 [details] [diff] [review]
Properly check if a WebM file has cues. r=

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

Looks good, thanks.  Can you submit this to the nestegg repo on github, and update this patch to be an import of the git repo using update.sh please?

::: media/libnestegg/include/nestegg.h
@@ +322,5 @@
> +/**
> + * Query the presence of cues.
> + * @retval 0 The media has no cues.
> + * @retval 1 The media has cues. */
> +int nestegg_has_cues(nestegg* context);

Space between nestegg and *.

::: media/libnestegg/src/nestegg.c
@@ +1587,5 @@
>    *tracks = ctx->track_count;
>    return 0;
>  }
>  
> +int nestegg_has_cues(nestegg * ctx)

New line after int.

@@ +1589,5 @@
>  }
>  
> +int nestegg_has_cues(nestegg * ctx)
> +{
> +  return ctx->segment.seek_head.head != 0;

There may be a SeekHead with no Cues entry, and it's also possible (if the Cues appear before the Clusters) for there to be no SeekHead at all.  So you want something like:

  return ctx->segment.cues.cue_point.head || ne_find_seek_for_id(ctx->segment.seek_head.head, ID_CUES);

Comment 6

[Paul Adenot (:padenot)]

I've addressed the comments, and submitted a pull request on github, waiting for you to merge before running |update.sh| and landing.

Comment 7

[Paul Adenot (:padenot)]

Attachment: Update libnestegg to last version.

Comment 8

[Paul Adenot (:padenot)]

Attachment: Properly check if a WebM file has cues.

I've split the libnestegg update patch (that I made using the update.sh script)
and the patch that call the new function.

Comment 9

[Matthew Gregan [:kinetik]]

Thanks!

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Paul, what's the ETA on landing this?

Comment 11

[Paul Adenot (:padenot)]

This is landing today.

Comment 12

[Paul Adenot (:padenot)]

The cubeb changes got landed by Steve Workman, so this is only the use of the new API, plus the symbol added to the list of symbols:

https://hg.mozilla.org/integration/mozilla-inbound/rev/34a9ef8f929d

Comment 13

[Ed Morley [:emorley]]

Push backed out for Windows pgo-only mochitest-1 timeouts in media tests, since the backout of just 1abf4c88f8f1 didn't work (see bug 793274 comment 12 for example logs):
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2912b7e727a

Comment 14

[Paul Adenot (:padenot)]

After bisecting the queue on try, this patch has been proved to cause no problem [0], note the patch at the top of the queue that requests a PGO build. The problem remains in the other part of the queue [1].

This has been pushed as https://hg.mozilla.org/integration/mozilla-inbound/rev/d33e1241da64

[0]: https://tbpl.mozilla.org/?tree=Try&rev=516ddf4c963c
[1]: https://tbpl.mozilla.org/?tree=Try&rev=418c581b8023

Comment 15

[Paul Adenot (:padenot)]

Also, https://hg.mozilla.org/integration/mozilla-inbound/rev/d33e1241da64 for the symbol change.

Comment 16

[Paul Adenot (:padenot)]

Hm, and https://hg.mozilla.org/integration/mozilla-inbound/rev/5f0038a23402 for the fix in the reader, since I seem to have made a copy paste error.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/5f0038a23402
https://hg.mozilla.org/mozilla-central/rev/d33e1241da64

Comment 18

[Paul Adenot (:padenot)]

Comment on attachment 699916 [details] [diff] [review]
Update libnestegg to last version.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 816949
User impact if declined: some webm video won't play
Testing completed (on m-c, etc.): baked for a long time on m-c
Risk to taking this patch (and alternatives if risky): little risk, fixes a regression. if not taken, some webm video won't play
String or UUID changes made by this patch: none

Comment 19

[Paul Adenot (:padenot)]

Comment on attachment 699917 [details] [diff] [review]
Properly check if a WebM file has cues.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 816949
User impact if declined: some webm video won't play
Testing completed (on m-c, etc.): baked for a long time on m-c
Risk to taking this patch (and alternatives if risky): little risk, fixes a regression. if not taken, some webm video won't play
String or UUID changes made by this patch: none

(nominating in reaction to the release-mgmt email)

Comment 20

[Paul Adenot (:padenot)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1fb834da769c
https://hg.mozilla.org/releases/mozilla-aurora/rev/e66e54290fe6

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this out for Windows bustage: https://hg.mozilla.org/releases/mozilla-aurora/rev/de299ce2b2c1

Comment 22

[Paul Adenot (:padenot)]

Relanded with the fix:

https://hg.mozilla.org/releases/mozilla-aurora/rev/36f3bc2e40bd
https://hg.mozilla.org/releases/mozilla-aurora/rev/4fd7553bfa40

Comment 23

[Paul Silaghi, QA [:pauly]]

The video works now on FF 20b1 Win 7, Ubuntu 12.04, Mac OS X 10.7.5

Comment 24

[Paul Silaghi, QA [:pauly]]

But is it known that FF/RW is not possible?

Comment 25

[Paul Adenot (:padenot)]

Yes, this video is bogus as noted by kinetik in comment 1. The fact that we can't seek (FF/RW) is expected, this bug was about not being able to play the video at all.

Comment 26

[Paul Silaghi, QA [:pauly]]

Thanks for explaining.

Comment 27

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 21b3 Win 7, Ubuntu 12.04 and Mac OS X 10.8.3