Closed Bug 578536 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Possibly related to Bug 583767.
(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.
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → chris
Attachment #490789 - Flags: review?(chris.double)
Blocks: 610570
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?
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.
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.
Attached patch Patch v2 (obsolete) — Splinter Review
* 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.
Attachment #490789 - Attachment is obsolete: true
Attachment #493121 - Flags: review?(chris.double)
Attachment #490789 - Flags: review?(chris.double)
(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 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.
Attachment #493121 - Flags: review?(chris.double) → review+
With count c/dtor and private copy constructor and assignment operator on NesteggPacketHolder. Thanks for the fast review doublec, carrying forward r=doublec.
Attachment #493121 - Attachment is obsolete: true
Attachment #493298 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/00ea82fd6c59
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.