mediasource duration not properly updated after an appendBuffer

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

Trunk
mozilla42
Points:
---

Firefox Tracking Flags

(firefox41 affected, firefox42 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Per spec:

"If the media segment contains data beyond the current duration, then run the duration change algorithm with new duration set to the maximum of the current duration and the group end timestamp."

currently, we set the mediasource duration to the end of the buffered range; which is the intersection of audio track and video track.
As such, we set the mediasource duration to min(audiotrack.duration, videotrack.duration) rather than max(audiotrack.duration, videotrack.duration)
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
(Assignee)

Comment 1

4 years ago
Created attachment 8625866 [details] [diff] [review]
Set mediasource duration to group end timestamp.
Attachment #8625866 - Flags: review?(karlt)
Comment on attachment 8625866 [details] [diff] [review]
Set mediasource duration to group end timestamp.

>+  virtual TimeUnit GroupEndTimestamp()
>+  {
>+    return Buffered().GetEnd();
>+  }

Please make the SourceBufferContentManager method pure virtual and move this
implementation to TrackBuffer.

>+  TimeUnit mOfficialGroupEndTimestamp;

Rather than having the same info stored in two different member variables,
which only differ during ProcessFrame(), please use a local variable in
ProcessFrame() together with a single member variable, and document the
thread-access of the member variable.  It is modified only in monitor on the
task queue, and so reading on other threads is in the monitor.
RestartGroupStartTimestamp() does not need to use the monitor because it is on
the same thread as the write.
Attachment #8625866 - Flags: review?(karlt) → review+
(Assignee)

Comment 3

4 years ago
(In reply to Karl Tomlinson (ni?:karlt back June 30) from comment #2)
> Comment on attachment 8625866 [details] [diff] [review]
> Set mediasource duration to group end timestamp.
> 
> >+  virtual TimeUnit GroupEndTimestamp()
> >+  {
> >+    return Buffered().GetEnd();
> >+  }
> 
> Please make the SourceBufferContentManager method pure virtual and move this
> implementation to TrackBuffer.

will do.

> 
> >+  TimeUnit mOfficialGroupEndTimestamp;
> 
> Rather than having the same info stored in two different member variables,
> which only differ during ProcessFrame(), please use a local variable in
> ProcessFrame() together with a single member variable, and document the
> thread-access of the member variable.  It is modified only in monitor on the
> task queue, and so reading on other threads is in the monitor.
> RestartGroupStartTimestamp() does not need to use the monitor because it is
> on
> the same thread as the write.

it can be accessed from the main thread in TrackBuffersManager::GroupEndTimestamp().

So if I only use a single mGroupEndTimestamp it would have to be protected by a monitor whenever it is written to..

mGroupEndTimestamp is modified in ProcessFrame upon every call, and as-such is modified very often.
Having to introduce locking inside ProcessFrame would be less than ideal it's already slow as it is :(

Maybe I could use a Mirror<TimeUnit> instead... but that still means two variables (including the Canonical)
(In reply to Jean-Yves Avenard [:jya] from comment #3)
> (In reply to Karl Tomlinson (ni?:karlt back June 30) from comment #2)
> > >+  TimeUnit mOfficialGroupEndTimestamp;
> > 
> > Rather than having the same info stored in two different member variables,
> > which only differ during ProcessFrame(), please use a local variable in
> > ProcessFrame() together with a single member variable, and document the
> > thread-access of the member variable.  It is modified only in monitor on the
> > task queue, and so reading on other threads is in the monitor.
> > RestartGroupStartTimestamp() does not need to use the monitor because it is
> > on
> > the same thread as the write.
> 
> it can be accessed from the main thread in
> TrackBuffersManager::GroupEndTimestamp().
> 
> So if I only use a single mGroupEndTimestamp it would have to be protected
> by a monitor whenever it is written to..

Just like mOfficialGroupEndTimestamp is.

> mGroupEndTimestamp is modified in ProcessFrame upon every call, and as-such
> is modified very often.
> Having to introduce locking inside ProcessFrame would be less than ideal
> it's already slow as it is :(

There is already locking.

https://hg.mozilla.org/integration/mozilla-inbound/annotate/78c1272f812f/dom/media/mediasource/TrackBuffersManager.cpp#l1218
(Assignee)

Comment 6

4 years ago
(In reply to Karl Tomlinson (ni?:karlt back June 30) from comment #5)
> (In reply to Jean-Yves Avenard [:jya] from comment #3)
> > (In reply to Karl Tomlinson (ni?:karlt back June 30) from comment #2)
> > > >+  TimeUnit mOfficialGroupEndTimestamp;
> > > 
> > > Rather than having the same info stored in two different member variables,
> > > which only differ during ProcessFrame(), please use a local variable in
> > > ProcessFrame() together with a single member variable, and document the
> > > thread-access of the member variable.  It is modified only in monitor on the
> > > task queue, and so reading on other threads is in the monitor.
> > > RestartGroupStartTimestamp() does not need to use the monitor because it is
> > > on
> > > the same thread as the write.
> > 
> > it can be accessed from the main thread in
> > TrackBuffersManager::GroupEndTimestamp().
> > 
> > So if I only use a single mGroupEndTimestamp it would have to be protected
> > by a monitor whenever it is written to..
> 
> Just like mOfficialGroupEndTimestamp is.

mGroupEndTimestamp is modified in the critical loop (TrackBuffersManager::ProcessFrame); there is no locking when accessing it as it's only ever accessed from the trackbuffer's task queue.

The monitor only protects mOfficialGroupEndTimestamp

Combining mGroupEndTimestamp and mOfficialGroupEndTimestamp means that you would need to introduce locking in TrackBuffersManager::ProcessFrame and you don't want to do that for performance reasons.


> 
> > mGroupEndTimestamp is modified in ProcessFrame upon every call, and as-such
> > is modified very often.
> > Having to introduce locking inside ProcessFrame would be less than ideal
> > it's already slow as it is :(
> 
> There is already locking.

yes, but that's only to protect mOfficialGroupEndTimestamp which is only modified once per media segment, compare to mGroupEndTimestamp which is modified for every frame processed.

A media segment typically contains about 300 frames, mGroupEndTimestamp is modified for every frame processed.

However, now that bug 1171760 has landed, what you suggest can be implemented without performance impact as we now process a group of frames at a time, vs frame by frame.
https://hg.mozilla.org/mozilla-central/rev/78c1272f812f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Target Milestone: mozilla41 → mozilla42
(In reply to Jean-Yves Avenard [:jya] from comment #6)
> (In reply to Karl Tomlinson (ni?:karlt back June 30) from comment #5)
> > (In reply to Jean-Yves Avenard [:jya] from comment #3)
> > > (In reply to Karl Tomlinson (ni?:karlt back June 30) from comment #2)
> > > > Rather than having the same info stored in two different member variables,
> > > > which only differ during ProcessFrame(), please use a local variable in
> > > > ProcessFrame()

> mGroupEndTimestamp is modified in the critical loop
> (TrackBuffersManager::ProcessFrame); there is no locking when accessing it
> as it's only ever accessed from the trackbuffer's task queue.

I'm asking for that usage of mGroupEndTimestamp to be replaced with a local variable, which is then saved to the member variable where you are setting mOfficialGroupEndTimestamp.
(Assignee)

Comment 9

4 years ago
(In reply to Karl Tomlinson (ni?:karlt) from comment #8)

> I'm asking for that usage of mGroupEndTimestamp to be replaced with a local
> variable, which is then saved to the member variable where you are setting
> mOfficialGroupEndTimestamp.

I understand what you're asking, but prior bug 1171760 was landed, it couldn't be done easily. because a local variable containing the group end time would have been out of scope before you got to where you update mOfficialGroupEndTimestamp.

ProcessFrame was called in a loop within CompleteCodedFrameProcessing, updating a local variable down there and pass it up to CompletedCodedFrameProcessing was no better than having a member variable I don't think.
You need to log in before you can comment on or make changes to this bug.