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)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: bechen, Assigned: jerry)
References
Details
Attachments
(2 files)
650 bytes,
text/html
|
Details | |
5.55 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 2•7 years ago
|
||
Yes.
Reporter | ||
Comment 3•7 years ago
|
||
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.
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(hshih)
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 4•7 years ago
|
||
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)
Assignee | ||
Comment 5•7 years ago
|
||
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/
Assignee | ||
Comment 6•7 years ago
|
||
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.
Comment 7•7 years ago
|
||
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 ;)
Comment 8•7 years ago
|
||
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).
Comment 9•7 years ago
|
||
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)
Assignee | ||
Comment 10•7 years ago
|
||
(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)
Comment 11•7 years ago
|
||
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)
Assignee | ||
Comment 12•7 years ago
|
||
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)
Assignee | ||
Comment 13•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fc3636e39dc728c2c4a8796bb31e0634cd73ee2
Updated•7 years ago
|
Attachment #8880690 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 14•7 years ago
|
||
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
Reporter | ||
Comment 15•7 years ago
|
||
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)
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/85099e5d1e2a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
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
status-firefox56:
fixed → ---
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.
Comment 20•7 years ago
|
||
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 ago → 7 years ago
status-firefox56:
--- → fixed
Flags: needinfo?(hshih)
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 22•7 years ago
|
||
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
Comment 23•7 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•