578536 - WebM decoder doesn't set {SoundData,VideoData}::mOffset

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

The WebM decoder backend doesn't fill SoundData::mOffset or VideoData::mOffset with valid values. *Data::mOffset is used to set nsBuiltinDecoder::mPlaybackPosition, whose value eventually ends up being used in nsHTMLMediaElement::UpdateReadyStateForData(). I think this means our calculation for whether we should switch readyState to HAVE_ENOUGH_DATA will be inaccurate for WebM.

Comment 1

[Joe Smith]

Possibly related to Bug 583767.

Comment 2

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

(In reply to comment #0)
> The WebM decoder backend doesn't fill SoundData::mOffset or VideoData::mOffset
> with valid values. *Data::mOffset is used to set
> nsBuiltinDecoder::mPlaybackPosition, whose value eventually ends up being used
> in nsHTMLMediaElement::UpdateReadyStateForData(). I think this means our
> calculation for whether we should switch readyState to HAVE_ENOUGH_DATA will be
> inaccurate for WebM.

I'm right, we do use *Data::mOffset to determine whether we should switch to readyState HAVE_ENOUGH_DATA, and this calculation is horribly optimistic for WebM.

Most of the time we won't notice, because most of the time the download is faster than the decode, so claiming to be in readyState HAVE_ENOUGH_DATA isn't a problem. When the download is slower than the decode, we start assuming we can play through, even though we may actually not be able to.

Comment 3

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

Attachment: Patch

This patch makes the offset in the {Sound,Video}Data output by nsWebMReader approximately correct. It's not exact, but approximate enough to enable our canplaythrough calculations to work well enough. I imagine being more exact would be a real hassle, and not really worth it.

Comment 4

[cajbir (:cajbir)]

The 'Tell' result will produce an offset beyond where the actual video data is (as you note about it being approximate). In what situations will this cause problems? Is the offset used anywhere else other than canplaythrough calculations? Can we get the actual offset through nestegg?

Comment 5

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

The playback offset is used in nsMediaDecoder::CanPlayThrough() to calculate whether the playback offset is likely to overtake the playback position. I couldn't see this value is used anywhere else.

If this calculation is wrong, we'll see problems like switching to HAVE_ENOUGH_DATA readyState too soon (or not switching to HAVE_ENOUGH_DATA until the entire resource is loaded). If we switch to HAVE_ENOUGH_DATA too soon, we will break out of buffering and resume playback, even though we can't really playback.

The specific problem I can reproduce is if I serve http://myrandomnode.dyndns.org:8080/~gmaxwell/portal2/Portal_2_3.webm at 400KB/s and let it play through (don't seek, that resets the playback offset) then around the 3 minute mark we'll start switching between HAVE_CURRENT_DATA and HAVE_ENOUGH_DATA, and so we'll be toggling between buffering and playing with high frequency. CanPlayThrough() thinks that the playback offset is still near the start of the media, and that 3/4 of the media is downloaded, so it thinks it can play through.

Comment 6

[Matthew Gregan [:kinetik]]

Call Tell() after nestegg_read_packet returns in nsWebMReader::NextPacket, and store the offset in a struct that wraps nestegg_packet.  The offset will be accurate because nestegg_read_packet returns immediately after reading a packet.

Comment 7

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

Attachment: Patch v2

* Create a container for nestegg_packets which holds the packet and its offset in the file.
* When we read a packet with nestegg_read_packet, grab the nsMediaStream offset and store the packet and its offset in the container. This should be the end offset of the packet (nestegg_read_packet() should stop reading at the end of the packet's Block element).
* Have nsWebMReader's packet queues store packet containers rather than raw packets.

These changes mean that we grab the stream offset when packets are read, rather than when they're decoded as in the previous patch.

Comment 8

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

(In reply to comment #7)
> Created attachment 493121 [details] [diff] [review]
> Patch v2

This corresponds to Matthew's suggestion. Test pass locally on Windows and in Linux VM.

Comment 9

[cajbir (:cajbir)]

Comment on attachment 493121 [details] [diff] [review]
Patch v2

>+// Holds a nestegg_packet, and its file offset. This is needed so we
>+// know the offset in the file we've played up to, in order to calculate
>+// whether it's likely we can play through to the end without needing
>+// to stop to buffer, given the current download rate.
>+class NesteggPacketHolder {
>+public:
>+  NesteggPacketHolder(nestegg_packet* aPacket, PRInt64 aOffset)
>+    : mPacket(aPacket), mOffset(aOffset) {}
>+  ~NesteggPacketHolder() {
>+    nestegg_free_packet(mPacket);
>+  }
>+  nestegg_packet* mPacket;
>+  // Offset in bytes. This is the offset of the end of the Block
>+  // which contains the packet.
>+  PRInt64 mOffset;
>+};

- Add MOZ_COUNT_DTOR and MOZ_COUNT_CTOR for leak tracking. 
- Declare (but don't implement) a private copy constructor and assignment operator to prevent copying so accidental copies don't result in double free of packets.

r+ with those changes.

Comment 10

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

Attachment: Patch 2: with review comments addressed

With count c/dtor and private copy constructor and assignment operator on NesteggPacketHolder. Thanks for the fast review doublec, carrying forward r=doublec.

Comment 11

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

http://hg.mozilla.org/mozilla-central/rev/00ea82fd6c59