TrackBuffersManager not always notifying demuxer of new data

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

4 years ago
The TrackBuffersManager demux all data as they are available.

It does so by appending the incoming data to a SourceBufferResource and then calling the MediaDataDemuxer .

However, it doesn't always notify the demuxer whenever the SourceBufferResources has been modified.

It isn't a problem with the MP4Demuxer has it doesn't rely on this information to know which data is available in the MediaResource ; however the WebM demuxer does which could lead to only being notified of partial and incomplete data.
Comment on attachment 8641487 [details] [diff] [review]
[MSE] Always notify demuxer when data is added (or removed) to the resource.

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

r+ with nits.

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +780,5 @@
> +{
> +  MOZ_ASSERT(mCurrentInputBuffer);
> +  int64_t offset = mCurrentInputBuffer->GetLength();
> +  mCurrentInputBuffer->AppendData(aData);
> +  mInputDemuxer->NotifyDataArrived(aData->Length(), offset);

Length() returns a size_t but NotifyDataArrived() takes a uint32_t there. I'm guessing an overflow is practically impossible, but doesn't the compiler complain?

@@ +1032,4 @@
>    MediaByteRange mediaRange = mParser->MediaSegmentRange();
>    uint32_t length;
>    if (mediaRange.IsNull()) {
>      length = mInputBuffer->Length();

'length' is not needed in this block anymore.

@@ +1037,4 @@
>      mInputBuffer = nullptr;
>    } else {
>      // The mediaRange is offset by the init segment position previously added.
>      length = mediaRange.mEnd - (mProcessedInput - mInputBuffer->Length());

Declare |uint32_t length| (or whatever type is appropriate) here and remove the declaration at line 1033 above.
Attachment #8641487 - Flags: review?(gsquelart) → review+
https://hg.mozilla.org/mozilla-central/rev/1e049536f283
Assignee: nobody → jyavenard
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.