Closed Bug 1374548 Opened 7 years ago Closed 7 years ago

The last frame issue of bipbop.mp4

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bechen, Assigned: jerry)

References

Details

Attachments

(2 files)

Nightly linux build.
When play the dom/media/test/bipbop.mp4 to the end, I see the frame on the screen is not the last frame, maybe it is the (last-1) frame shown on the screen.
same as bug 1346156?
See Also: → 1346156
One interesting thing is that in the attach file, I append a textNode to the MediaElement, the video shows the last frame(296) in the screen, then mouse click, the last 2nd frame(295) shown.
Flags: needinfo?(hshih)
Priority: -- → P3
When the video is not playing, the layer status becomes LAYER_INACTIVE[1]. So, gecko will use BasicLayerManager to draw this video layer.

In BasicImageLayer, gecko will always draw the first frame in image container[2]. That's why we see the roll-back frame on the screen.

The solution will be:
a) Always use LAYER_INACTIVE at [1].
b) Always show the last frame in image container at [2].
c) Create a new message to passing the shown frame_id from compositor to content. Then, gecko could use this info to selection the frame.

I think the c) is too complicated.

The a) will increase the overhead for the non-compositing-cheap compositor.

The b) solution is weird. I don't know if there is another path uses basicLayer with multiple frame in image container. If yes, this solution might break that path.

[1]
https://dxr.mozilla.org/mozilla-central/rev/92eb911c35da48907d326604c4c92cf55e551895/layout/generic/nsVideoFrame.cpp#467
[2]
https://dxr.mozilla.org/mozilla-central/rev/92eb911c35da48907d326604c4c92cf55e551895/gfx/layers/basic/BasicImageLayer.cpp#75
Assignee: nobody → hshih
Status: NEW → ASSIGNED
Flags: needinfo?(hshih) → needinfo?(matt.woodrow)
typo(In reply to Jerry Shih[:jerry] (UTC+8) from comment #4)
> The solution will be:
> a) Always use LAYER_INACTIVE at [1].

typo.
s/LAYER_INACTIVE/LAYER_ACTIVE/
Another solution is that make the decoder just send 1 frame at end.

The decoder could decode multiple frames at the same time. If the decoder just decodes 1 frame at the end, there is no frame selection problem here.
The decoder never knows in advance that what it's decoding is the last frame.

The MediaDecoderStateMachine however, attempts to keep about 10 decoded video frames ahead of time.

Having said that, I don't see how this helps the last frame. It's the last one after all ;)
Wrong terms are used.

What Jerry means is VideoSink should send exactly only one frame to the compositor in the last render loop to ensure the BasicLayerManager will not pick the wrong frame to render (BasicLayerManager always picks the first one which is not desired at the end of playback).
Why can't BasicLayerManager have the same logic to pick the frame to display, based on current time?

If the current time is at the end, then we should be able to know that, and display the last frame.
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> Why can't BasicLayerManager have the same logic to pick the frame to
> display, based on current time?
> 
> If the current time is at the end, then we should be able to know that, and
> display the last frame.

There is no composition time at client side[1]. If we try to pass composition time from parent to child, this method is similar to the c) in comment 4. Matt, do you think we could just use the "current time" to select the frame in image container?

[1]
https://dxr.mozilla.org/mozilla-central/rev/f8bb96fb5c4f9960c5f754f877eceb677df18ddc/gfx/layers/composite/ImageComposite.cpp#86
Flags: needinfo?(matt.woodrow)
The composition time is just TimeStamp::Now(), we just compute it at the start of the frame, or use the value given to us from vsync.

It seems like BasicLayerManager can just lookup TimeStamp::Now() too.
Flags: needinfo?(matt.woodrow)
The BasicImageLayer always picks the first frame in image container which is not desired for video playback.

In this patch, the composition time will be set in BasicLayerManager::EndTransaction(). Then, gecko will use
that time to selection the proper image frame in image container.
Since the BasicLayerManager is created on demand, it might have multiple BasicLayerManager in one frame updating.
So, the composition time between each BasicLayerManager in the frame will not be the same. If this is a big
problem, we should find another place for the composition time storing.
Attachment #8880690 - Flags: review?(matt.woodrow)
Attachment #8880690 - Flags: review?(matt.woodrow) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85099e5d1e2a
Implement the frame selection in BasicImageLayer. r=mattwoodrow
Keywords: checkin-needed
Jerry:
Do you have plan to add a testcase for it? Or I can write a reftest to verify the last frame of bipbop.mp4.
Flags: needinfo?(hshih)
Please help to do that.
Flags: needinfo?(hshih)
https://hg.mozilla.org/mozilla-central/rev/85099e5d1e2a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I had to back this out for being the likely cause of the recent spike of crashes from bug 1370089:
https://hg.mozilla.org/mozilla-central/rev/abd8809269675a9c5efb6778b19ba6e6607f626d
Status: RESOLVED → REOPENED
Flags: needinfo?(hshih)
Resolution: FIXED → ---
Target Milestone: mozilla56 → ---
Seems this wasn't at fault for the spike in crashes, I'll reland this in a bit.
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/57b57379c60c
Implement the frame selection in BasicImageLayer. r=mattwoodrow a=relanding
Sorry for the churn here.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: needinfo?(hshih)
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Looks like that the landing of this bug caused permanent orange of test_bug360437.xul.

There is same assertion count mismatch orange bug as bug 1321173. However, the caused assertion of the intermittent orange was:

> ###!!! ASSERTION: Texture initialization failed! -- error 0x506, Source 0, Source format 6408,  RGBA Compat 1: 'Error', file /builds/slave/m-in-m64-d-0000000000000000000/build/src/gfx/layers/opengl/CompositorOGL.cpp, line 792

However, after landing the patch, there is new permanent orange due to an NS_ASSERTION:

>  ###!!! ASSERTION: input context of focused widget should be set from a remote process: 'aWindow->GetInputContext().IsOriginContentProcess()', file c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/widget/windows/WinIMEHandler.cpp, line 402
Ah, the regression point may be different. This change set has the orange at Win-x64 debug build:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=abd8809269675a9c5efb6778b19ba6e6607f626d

Sorry for the spam.
Blocks: 1378285
You need to log in before you can comment on or make changes to this bug.