WebM buffered: should use duration for end time of last buffered range

RESOLVED FIXED in mozilla15

Status

()

Core
Audio/Video
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: kinetik, Assigned: padenot)

Tracking

Trunk
mozilla15
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [wip])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
The existing code contains a special case that uses the duration for the end time of the range if the entire file is cached.  We should also do this when the last range extends to the end of the file.

Patched based on top of bug 598217.  Will need to be rebased on top of bug 	598242 before review and landing.
(Reporter)

Comment 1

7 years ago
Created attachment 478154 [details] [diff] [review]
patch v0
(Reporter)

Comment 2

7 years ago
Oh, also:

  // Special case completely cached files.  This also handles local files.
  if (stream->IsDataCachedToEndOfStream(0)) {
    uint64_t duration = 0;
    if (nestegg_duration(mContext, &duration) == 0) {
      aBuffered->Add(aStartTime / MS_PER_S, duration / NS_PER_S);
    }

This case should be changed to fall through to trying to use CalculateBufferedForRange if the file has no duration.
(Reporter)

Comment 3

7 years ago
Also also, if the file is local (i.e. a nsMediaFileStream) and has no duration, we'll report an empty buffered range for it.  Not sure how best to handle that.
(Reporter)

Updated

7 years ago
Whiteboard: [wip]
Blocks: 559468
Assignee: kinetik → paul
Created attachment 623374 [details] [diff] [review]
patch v1 : address comment 2 and 3 and rebasing.

We should probably have a test, here. I'm wondering if we have a webm file without a duration in the files available in content/media/test/ (mkvinfo reports a duration for all the files we have, but I'm not sure if it uses the header or if it reads the whole file).

If we have no such file, we need to craft one to test this new code reliably. I was not able to remote the duration info from a regular webm file using the mkvpropedit program, and haven't got around learning the matroska bitstream yet to modify one by hand. Any thoughts ?
Attachment #478154 - Attachment is obsolete: true
Attachment #623374 - Flags: review?(kinetik)
(Reporter)

Comment 5

5 years ago
Created attachment 623559 [details]
seek.webm with duration removed

(In reply to Paul ADENOT (:padenot) from comment #4)
> We should probably have a test, here. I'm wondering if we have a webm file
> without a duration in the files available in content/media/test/ (mkvinfo
> reports a duration for all the files we have, but I'm not sure if it uses
> the header or if it reads the whole file).

Here's one.  It's identical to seek.webm except that the duration has been disabled.  I'm not aware of a tool that'll produce files like this, so I created this file by hex editing seek.webm to convert the duration element into a void element, which the WebM parser will skip over.

FWIW, we don't read the entire file.  For WebM we rely on the duration element being present, since non-live duration-less WebM files should be extremely rare.  For Ogg files, we seek to the end and search for the duration if it's not present in the Ogg Index.
(Reporter)

Comment 6

5 years ago
The patch is removing the buffered range normalization added in bug 598242, which we don't want to do.  We *should* have a test that the buffered ranges are normalized, but I'm don't think we do.  It'd be a simple check to add to test_buffered if we had a WebM file that started at a non-zero offset.
(In reply to Matthew Gregan [:kinetik] from comment #6)
> The patch is removing the buffered range normalization added in bug 598242,
> which we don't want to do.  We *should* have a test that the buffered ranges
> are normalized, but I'm don't think we do.  It'd be a simple check to add to
> test_buffered if we had a WebM file that started at a non-zero offset.

I think I've handled this case :

> +        double startTime = start * timecodeScale / NS_PER_S - aStartTime;
> +        double endTime = end * timecodeScale / NS_PER_S - aStartTime;

it has merely been moved outside of the CalculateBufferedForRange method. I can put it back again if you feel it would be better

fwiw, if I play the |split.webm| (which starts at a non-zero offset) file with the patch applied, it plays from 0 to 1.967, and buffered is [0, 1.967] (with the same file, mplayer reports a playback between 2 and 3.967).

There is already a test for that in test_buffered.html, because it uses the |split.webm| file (and it passes, with and without this patch, that is, the ranges are normalized correctly).
(Reporter)

Comment 8

5 years ago
Comment on attachment 623374 [details] [diff] [review]
patch v1 : address comment 2 and 3 and rebasing.

(In reply to Paul ADENOT (:padenot) from comment #7)
> I think I've handled this case :
> 
> > +        double startTime = start * timecodeScale / NS_PER_S - aStartTime;
> > +        double endTime = end * timecodeScale / NS_PER_S - aStartTime;

Indeed, my mistake, sorry!
Attachment #623374 - Flags: review?(kinetik) → review+
I found a problem. When we are streaming a media that has no duration, in a live fashion, and when the download ends, a duration is set (i.e. is reported to be different from Infinity [1]), and it is different from the end of the |buffered| ranges.

|nsBuiltinDecoderStateMachine::UpdatePlaybackPositionInternal| indeed updates |mEndTime| with a lowest duration (by a very small amount, usually on the order of 0.001s) than the number we get from the WebM parsing.

Do we want to address this case, or is it a situation that is too rare for us to care ? Maybe it is better to land that patch as it is to allow bug 559468 to land, and maybe to address that problem in a follow up.

[1] : http://mxr.mozilla.org/mozilla-central/source/content/media/nsBuiltinDecoder.cpp#594
(Reporter)

Comment 10

5 years ago
Let's deal with that in a follow up.  It's not high priority, there are quite a few rough edges with live streams we need to work through.
Created attachment 623941 [details] [diff] [review]
(Added name a commit message).
Attachment #623374 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/7da36f8c159a
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/7da36f8c159a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

3 years ago
Depends on: 1072483
You need to log in before you can comment on or make changes to this bug.