1177147 - mediasource duration not properly updated after an appendBuffer

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

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)

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Set mediasource duration to group end timestamp.

Comment 2

[Karl Tomlinson (: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.

Comment 3

[Jean-Yves Avenard [:jya]]

(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)

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78c1272f812f

Comment 5

[Karl Tomlinson (:karlt)]

(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

Comment 6

[Jean-Yves Avenard [:jya]]

(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.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/78c1272f812f

Comment 8

[Karl Tomlinson (:karlt)]

(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.

Comment 9

[Jean-Yves Avenard [:jya]]

(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.