Open Bug 1291946 Opened 3 years ago Updated Last year

Intermittent dom/media/test/test_streams_element_capture_createObjectURL.html | 320x240.ogv checking readyState - got +0, expected 2, Check video frame pixel has been drawn - got +0, expected 255

Categories

(Core :: Audio/Video: Playback, defect, P3)

defect

Tracking

()

REOPENED
Tracking Status
firefox50 --- unaffected
firefox51 --- affected

People

(Reporter: intermittent-bug-filer, Assigned: ctai)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 1 obsolete file)

Component: Audio/Video → Audio/Video: Playback
fallout from bug 1201363?
Depends on: 1201363
The bug 1201363 is landed in 2016-08-04 07:16:03 PDT.
And this bug is reported before it.
So I don't think bug 1201363 is related to this bug.
But if you have more evidence, feel free to add the dependency. 
see: https://bugzilla.mozilla.org/show_bug.cgi?id=1201363#c178
No longer depends on: 1201363
Comment on attachment 8779689 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

https://reviewboard.mozilla.org/r/70640/#review67998

::: dom/media/MediaStreamGraph.cpp:2795
(Diff revision 1)
> -        videoSink->SetCurrentFrames(*(static_cast<VideoSegment*>(streamTrack->GetSegment())));
> +        // In some cases, the mTracks might contain empty video
> +        // segment, but the mUpdateTracks contain latest video segment
> +        // data.
> +        VideoSegment* videoSegment = static_cast<VideoSegment*>(streamTrack->GetSegment());
> +        if (videoSegment->IsEmpty() && found) {
> +          videoSegment = static_cast<VideoSegment*>(data->mData.get());
> +        }
> +        videoSink->SetCurrentFrames(*videoSegment);

Instead of having separate paths here I think we should join the chunks of both these segments in a new one and pass that to the listener.
Attachment #8779689 - Flags: review?(pehrson)
Comment on attachment 8779689 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

https://reviewboard.mozilla.org/r/70640/#review68118
Attachment #8779689 - Flags: review?(rjesup)
Comment on attachment 8779689 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

https://reviewboard.mozilla.org/r/70640/#review68228

::: dom/media/MediaStreamGraph.cpp:2795
(Diff revisions 1 - 2)
> -        // In some cases, the mTracks might contain empty video
> -        // segment, but the mUpdateTracks contain latest video segment
> -        // data.
> +        // In some cases, the mTracks might contain empty video segment, but
> +        // segment, but the mUpdateTracks contain latest video segment data. So
> +        // we append the latest video frames from mUpdateTracks.

I guess the empty video segment is now irrelevant. We just want to ensure all the available data gets set.

::: dom/media/MediaStreamGraph.cpp:2800
(Diff revisions 1 - 2)
> -        // In some cases, the mTracks might contain empty video
> -        // segment, but the mUpdateTracks contain latest video segment
> -        // data.
> +        // In some cases, the mTracks might contain empty video segment, but
> +        // segment, but the mUpdateTracks contain latest video segment data. So
> +        // we append the latest video frames from mUpdateTracks.
>          VideoSegment* videoSegment = static_cast<VideoSegment*>(streamTrack->GetSegment());
> -        if (videoSegment->IsEmpty() && found) {
> -          videoSegment = static_cast<VideoSegment*>(data->mData.get());
> +        if (found) {
> +          videoSegment->AppendFrom(data->mData.get());

AppendFrom will empty out the argument per [1].

I think we want to create an empty VideoSegment and copy both segments into it, so we don't put the ExtractPendingInput() algorithm in charge of shuffling this data out of play.

It might, now or in the future, want to do more things depending on the data available.


[1] http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/dom/media/MediaSegment.h#412
Attachment #8779689 - Flags: review?(pehrson)
Comment on attachment 8779689 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

https://reviewboard.mozilla.org/r/70640/#review68278

<p>Please fix the commit message since there's not really a race between AppendToTrack and ExtractPendingInput going on here.</p>

::: dom/media/MediaStreamGraph.cpp:2795
(Diff revision 3)
> -        videoSink->SetCurrentFrames(*(static_cast<VideoSegment*>(streamTrack->GetSegment())));
> +        // In some cases, the mUpdateTracks might contain latest video segment
> +        // data. So we append the latest video frames from
> +        // mUpdateTracks.

I think you can drop the comment. Or change it to say that we have to account for data appended but not handled by MSG yet.

::: dom/media/MediaStreamGraph.cpp:2799
(Diff revision 3)
> +        StreamTime startTime = GraphTimeToStreamTime(mGraph->mProcessedTime);
> +        StreamTime endTime = streamTrack->GetSegment()->GetDuration();
> +        videoSegment.AppendSlice(*streamTrack->GetSegment(), startTime, endTime);

You can now inline the start and end times.
Attachment #8779689 - Flags: review?(pehrson) → review+
Comment on attachment 8779689 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

https://reviewboard.mozilla.org/r/70640/#review68292
Attachment #8779689 - Flags: review?(rjesup) → review+
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #12)
> Comment on attachment 8779689 [details]
> Bug 1291946 - Append the latest video frames from updateTracks. .
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/70640/diff/3-4/

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a2b0c3e859a&selectedJob=25624166
Comment on attachment 8779689 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

Hi, Andreas,
You might want to review it again.
Attachment #8779689 - Flags: review+ → review?(pehrson)
Assignee: nobody → ctai
Comment on attachment 8779689 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

https://reviewboard.mozilla.org/r/70640/#review68708

::: dom/media/MediaStreamGraph.cpp:2798
(Diff revision 4)
> +        for (VideoSegment::ConstChunkIterator c(*streamTrackSegment); !c.IsEnded(); c.Next()) {
> +          if (c->mFrame.GetImage() != nullptr) {
> +            videoSegment.AppendFrame(do_AddRef(c->mFrame.GetImage()), c->GetDuration(),
> +                                               c->mFrame.GetIntrinsicSize(), c->GetPrincipalHandle(), c->mFrame.GetForceBlack(), c->GetTimeStamp());

I think we only want to skip null frames that come before the first non-null frame.

I don't know if we have null frames in the middle of a regular MediaSegment but in theory we could.

This is why AppendSlice was nice, if we just pass it the right time :-)

::: dom/media/MediaStreamGraph.cpp:2801
(Diff revision 4)
> +        VideoSegment* streamTrackSegment = static_cast<VideoSegment*>(streamTrack->GetSegment());
> +        // Append non-null video frames.
> +        for (VideoSegment::ConstChunkIterator c(*streamTrackSegment); !c.IsEnded(); c.Next()) {
> +          if (c->mFrame.GetImage() != nullptr) {
> +            videoSegment.AppendFrame(do_AddRef(c->mFrame.GetImage()), c->GetDuration(),
> +                                               c->mFrame.GetIntrinsicSize(), c->GetPrincipalHandle(), c->mFrame.GetForceBlack(), c->GetTimeStamp());

This looks too long.
Attachment #8779689 - Flags: review?(pehrson) → review+
Comment on attachment 8781010 [details]
Bug 1291946 - Call mozCapturedStreamXXX in onloadedmetadata callback. .

https://reviewboard.mozilla.org/r/71516/#review69032

::: dom/media/test/test_streams_element_capture_createObjectURL.html:57
(Diff revision 2)
>    v.play();
> +  v.onloadedmetadata = function () {
> +    stream = v.mozCaptureStreamUntilEnded();
> +    is(stream.currentTime, 0, test.name + " stream initial currentTime");
> +    vout.src = URL.createObjectURL(stream);
> -  vout.play();
> +    vout.play();
> +  };

The most important thing is that you do v.play() at the same time as mozCaptureStreamUntilEnded(), otherwise you're still at risk of v reaching the end before content reaches vout.

You'll also have to set v.preload = "metadata" since we by default don't preload on Android IIRC.
Attachment #8781010 - Flags: review?(pehrson)
Comment on attachment 8781010 [details]
Bug 1291946 - Call mozCapturedStreamXXX in onloadedmetadata callback. .

https://reviewboard.mozilla.org/r/71516/#review69048
Attachment #8781010 - Flags: review?(pehrson) → review+
Attachment #8779689 - Attachment is obsolete: true
Comment on attachment 8781391 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

Carry r+...Get some review board issue when I trying to fixed the patch title in the second patch.
Attachment #8781391 - Flags: review?(rjesup)
Attachment #8781391 - Flags: review?(pehrson)
Attachment #8781391 - Flags: review+
Comment on attachment 8781391 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

https://reviewboard.mozilla.org/r/71850/#review69358

::: dom/media/MediaStreamGraph.cpp:2796
(Diff revision 1)
>      if (foundTrack) {
>        MediaStreamVideoSink* videoSink = listener->AsMediaStreamVideoSink();
>        // Re-send missed VideoSegment to new added MediaStreamVideoSink.
>        if (streamTrack->GetType() == MediaSegment::VIDEO && videoSink) {
> -        videoSink->SetCurrentFrames(*(static_cast<VideoSegment*>(streamTrack->GetSegment())));
> +        VideoSegment videoSegment;
> +        videoSegment.AppendSlice(*streamTrack->GetSegment(), mTracks.GetForgottenDuration(), streamTrack->GetSegment()->GetDuration());

Line length.
Attachment #8781391 - Flags: review+
Comment on attachment 8781391 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

https://reviewboard.mozilla.org/r/71850/#review69410
Attachment #8781391 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e24eb751c21
Append the latest video frames from updateTracks. r=jesup, r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/872c1ec4d7ad
Call mozCapturedStreamXXX in onloadedmetadata callback. r=pehrsons
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/872c1ec4d7ad
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(In reply to OrangeFactor Robot from comment #34)
> 7 automation job failures were associated with this bug in the last 7 days.
> 
> Repository breakdown:
> * fx-team: 4
> * autoland: 2
> * mozilla-inbound: 1
> 
> Platform breakdown:
> * linux64: 6
> * windows8-64: 1
> 
> For more details, see:
> https://brasstacks.mozilla.com/orangefactor/
> ?display=Bug&bugid=1291946&startday=2016-08-15&endday=2016-08-21&tree=all

No failure after 8/18. Let's monitor it.
Flags: needinfo?(ctai)
Looks like only the test change has successfully landed. See comment 32.
Is this bug prematurely closed?
Flags: needinfo?(ctai)
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #36)
> Looks like only the test change has successfully landed. See comment 32.
> Is this bug prematurely closed?

Thanks for the reminder. I didn't notice only the test case landed. The test case change can avoid it. So I think we can close this bug. But the fix of the MSG code should also be landed. I will file another bug to fix it.
Flags: needinfo?(ctai)
This failure is still happening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla51 → ---
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
(In reply to Wes Kocher (:KWierso) from comment #32)
> Backed out for mda1 crashes on android debug
> https://treeherder.mozilla.org/logviewer.html#?job_id=33941080&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1eaa34d5dba1
I know what's going on now.
The crash is caused by " Assertion failure: aStart <= aEnd (Endpoints inverted)".
Because in gUM case(see [1]), the duration is delta(usually 10ms). So once the gUM case go to this code block the assertion happened. 

[1]: http://searchfox.org/mozilla-central/source/dom/media/webrtc/MediaEngineCameraVideoSource.cpp#19
Attachment #8781010 - Attachment is obsolete: true
Comment on attachment 8781391 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

I fixed the problem caused backout.
@Andreas, you might want to review it again. :)
Attachment #8781391 - Flags: review+ → review?(pehrson)
Attachment #8781010 - Attachment is obsolete: false
Comment on attachment 8781391 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

https://reviewboard.mozilla.org/r/71850/#review74810

While you're at it, change the `while` loop to `for` so we keep all the loop controls in one place. Then you also only need the `continue` statement.
Attachment #8781391 - Flags: review?(pehrson)
Comment on attachment 8781391 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

Carry previous r+. Comment 45 is for bug 1298305. I mixed two patches. :(
Attachment #8781391 - Flags: review?(pehrson) → review+
Patch "Bug 1291946 - Call mozCapturedStreamXXX in onloadedmetadata callback." is already landed.  Please land "Bug 1291946 - Append the latest video frames from updateTracks." only. This patch should be able to fix remaining intermittent test failures.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cbf25d6711b7
Append the latest video frames from updateTracks. r=jesup.
Keywords: checkin-needed
Comment on attachment 8781391 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

https://reviewboard.mozilla.org/r/71850/#review74996

::: dom/media/encoder/TrackEncoder.cpp:283
(Diff revision 3)
>    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
>  
>    // Append all video segments from MediaStreamGraph, including null an
>    // non-null frames.
>    VideoSegment::ChunkIterator iter(const_cast<VideoSegment&>(aSegment));
>    while (!iter.IsEnded()) {

I intentionally cleared the r? last time because I wanted to see the for loop here first. Without it you can't remove the iter.Next() like below.

::: dom/media/VideoSegment.h:95
(Diff revision 4)
> +  TimeStamp GetTimeStamp() const {
> +    return mTimeStamp;
> +  }
> +

I guess this is not needed?
Attachment #8781391 - Flags: review-
Arrr.... I messed the solution of bug 1298305 in https://reviewboard.mozilla.org/r/71850/#review74810.
The fix of while loop issue will be fixed in bug 1298305.
The back out is caused by other reason. I might know the root cause of the backout now.  

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #51)
> Comment on attachment 8781391 [details]
> Bug 1291946 - Append the latest video frames from updateTracks. .
> 
> https://reviewboard.mozilla.org/r/71850/#review74996
> 
> ::: dom/media/encoder/TrackEncoder.cpp:283
> (Diff revision 3)
> >    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> >  
> >    // Append all video segments from MediaStreamGraph, including null an
> >    // non-null frames.
> >    VideoSegment::ChunkIterator iter(const_cast<VideoSegment&>(aSegment));
> >    while (!iter.IsEnded()) {
> 
> I intentionally cleared the r? last time because I wanted to see the for
> loop here first. Without it you can't remove the iter.Next() like below.
> 
> ::: dom/media/VideoSegment.h:95
> (Diff revision 4)
> > +  TimeStamp GetTimeStamp() const {
> > +    return mTimeStamp;
> > +  }
> > +
> 
> I guess this is not needed?
Right, sorry, I missed that the code was unrelated to this bug :-)
Comment on attachment 8781391 [details]
Bug 1291946 - Append the latest video frames from updateTracks. .

https://reviewboard.mozilla.org/r/71850/#review75002
Attachment #8781391 - Flags: review- → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d79f2ef22ec4
Append the latest video frames from updateTracks. r=pehrsons
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d79f2ef22ec4
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
This is still happening :(
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1291946
Status: RESOLVED → REOPENED
Flags: needinfo?(ctai)
Resolution: FIXED → ---
Target Milestone: mozilla51 → ---
I will try to fix it.
Flags: needinfo?(ctai)
You need to log in before you can comment on or make changes to this bug.