Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable dirty regions for full screen video #6090

Merged
merged 6 commits into from
Jan 23, 2015

Conversation

FernetMenta
Copy link
Contributor

Moves video rendering (fullscreen) from application to GUIWindowFullScreen. By doing this dirty regions work on fullscreen mode and the platform has separate layers for video and ui.

@popcornmix iirc you still need some options to limit fps of gui, right?

@fritsch
Copy link
Member

fritsch commented Jan 2, 2015

When watching LiveTV in preview window - there is no picture at all.

When switching over to fullscreen it works as expected, when switching back - the last image stays on screen not updated.

@FernetMenta
Copy link
Contributor Author

@fritsch VideoControl is fixed now.

@fritsch
Copy link
Member

fritsch commented Jan 2, 2015

Yeah, I expected a one liner - works as expected. Btw. did you check with intel_gpu_top or something that we get measurable results? E.g. reduced gpu usage? More throughput?

Btw. @smallint wants to contact you for some specialities the cubox (and most likely other hw) will need, having a high performance fb on fb1 and another one fb0 which needs quite heavy operations to swap and sync.

@fritsch
Copy link
Member

fritsch commented Jan 2, 2015

Wait, something else.

The first channel after a "stop" does not show a picture. When switching to fullscreen and back, picture is there. Subsequent channels work if you don't press stop between them.

@FernetMenta
Copy link
Contributor Author

fixed again.
This change has no performance impact on systems with only a single buffer for rendering. Systems like Pi or Cubux will benefit from it.

@smallint
Copy link
Contributor

smallint commented Jan 2, 2015

I checked this PR and it seems to work regarding eglSwapBuffers which is not called anymore in fullscreen if nothing is visible. I incorporated the change needed in CLinuxRendererGLES::HasFrame() as discussed with @FernetMenta.

But if I press enter and the OSD should show up only the currenly updated controls are visible. Same is true for 'o'. Since the renderer calls glClear all controls are removed but not the onces currently dirty. Furthermore m_rotatedDestCoords in RenderIMXMAPTexture are somehow wrong? In LiveTV I get now values like "0 1082726400 0 1080074240" for "x0 y0 x1 y1" if displayed not fullscreen. In fullscreen I get always "0 0 0 0".

EDIT: Regarding m_rotatedDestCoords: my fault, I used the wrong printf specifier ;) It works as usual.

@FernetMenta
Copy link
Contributor Author

what dirty regions algo did you choose? in general we render all controls if there is only a single one dirty because there is no preserve back buffer.

@smallint
Copy link
Contributor

smallint commented Jan 2, 2015

 <gui>
    <algorithmdirtyregions>1</algorithmdirtyregions>
    <nofliptimeout>1000</nofliptimeout>
  </gui>

@smallint
Copy link
Contributor

smallint commented Jan 2, 2015

If I open the OSD and move with left and right through the buttons I can see only the currently updated buttons but all others are cleared.

@fritsch
Copy link
Member

fritsch commented Jan 2, 2015

@FernetMenta: The last squash patch needs #ifdef HAS_VIDEO_PLAYBACK

2015-01-02 13:10 GMT+01:00 smallint notifications@github.com:

If I open the OSD and move with left and right through the buttons I can
see only the currently updated buttons but all others are cleared.

Reply to this email directly or view it on GitHub
#6090 (comment).

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

@FernetMenta
Copy link
Contributor Author

@smallint algo is wrong. as you observe, your system has no back buffer preserve. hence you need to invalidate all controls.

@fritsch I think i will remove this outdated HAS_VIDEO_PLAYBACK in the near future. complete nonsense in this context here. because this is the VideoControl :)

@fritsch
Copy link
Member

fritsch commented Jan 2, 2015

@FernetMenta Might be, but it does not link without that :-) I did not care about the semantics.

@FernetMenta
Copy link
Contributor Author

it links here.

@fritsch
Copy link
Member

fritsch commented Jan 2, 2015

Okay, try make clean and full make again, then you might see.

2015-01-02 13:27 GMT+01:00 Rainer Hochecker notifications@github.com:

it links here.

Reply to this email directly or view it on GitHub
#6090 (comment).

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

@smallint
Copy link
Contributor

smallint commented Jan 2, 2015

@FernetMenta Actually the system has back buffer preserve. I checked it with another application and also the log does not tell otherwise. The renderer always calls glClear, doesn't it? And that destroys the back buffer anyhow.

I tried with "3" and it works that all controls are rendered if invalidated. But the next frame will remove them again completely (glClear?).

@smallint
Copy link
Contributor

smallint commented Jan 2, 2015

I could fix it for me with

if (m_bRenderBypass)
{
  ...
  g_renderManager.Render(m_bHasRendered, 0, 255);
  ...
}

Then it is working with both algos.

@popcornmix
Copy link
Member

@FernetMenta We do enable back buffer preserve (#3021) and the Pi certainly respects that flag.
I'll check if algorithmdirtyregions=1 works with video with this PR (it used to leave artefacts when video had black bars).

@Memphiz
Copy link
Member

Memphiz commented Jan 2, 2015

seems to work normal for osx and ios (just tested the osd controls and dialogs ...)

@popcornmix
Copy link
Member

@smallint where were you adding that code?

@FernetMenta
Copy link
Contributor Author

@popcornmix CGUIWindowFullScreen::FrameMove()
@smallint thanks for the fix.

@smallint
Copy link
Contributor

smallint commented Jan 2, 2015

@popcornmix In GUIWindowFullScreen.cpp:555

@smallint
Copy link
Contributor

smallint commented Jan 2, 2015

Now playback with and without GUI works very well and with almost full performance (no GUI). Next small issue is overlay rendering such as teletext. It only updates were OSD controls are dirty.

EDIT: Also 'o' does not remove the stats overlay completely. m_bHasRendered should take into account that the overlay was hidden to force clearing the back buffer. Probably there are some other controls which need such tracking?

@popcornmix
Copy link
Member

@smallint do videos with black bars clear background correctly?
I find UI shows in black bars with algorithmdirtyregions=1 on Pi.
Also I get artefacts on other screens like system info with algorithmdirtyregions=1 (looks to be an issue with alpha being applied more than once).

@smallint
Copy link
Contributor

smallint commented Jan 2, 2015

@popcornmix Yes. E.g. videos with 4:3 ratio do show correctly. I must admit that I am clearing the background manually if the cropping rect changes. With "background" I refer to another framebuffer device than the GL context uses. It is probably similar to the Pi hardware layering. In the GL context I just cut a hole with alpha=0 into the paint area if not fullscreen.

@FernetMenta
Copy link
Contributor Author

@smallint does the teletext dialog set its dirty flag? Maybe you need to override the DoProcess method and call MarkDirtyRegion when something has changed.

@smallint
Copy link
Contributor

smallint commented Jan 2, 2015

Where is the teletext dialog implemented? I will check that.

@FernetMenta
Copy link
Contributor Author

@smallint
Copy link
Contributor

smallint commented Jan 2, 2015

Found it. Indeed, it does not set the dirty flag. Doing that helps but "esc" for hiding it has then a very high latency then. I need to press esc twice to make it disappear after a couple of seconds.

@FernetMenta
Copy link
Contributor Author

how did you set it? dependent on m_TextDecoder.NeedRendering()?

@smallint
Copy link
Contributor

smallint commented Jan 2, 2015

Yes, but I forgot to check m_bClose in DoProcess. Now it is working.

@FernetMenta
Copy link
Contributor Author

where exactly do you read this from? please point me to the lines in the log

@smallint
Copy link
Contributor

Not from this logs. I just read it previously. Here it another log where the first number after "r:" is the buffer index: http://xbmclogs.com/show.php?id=399113.

@smallint
Copy link
Contributor

I captured now two cases and logged the RenderManager states as well: http://xbmclogs.com/show.php?id=399132. Just for completeness. It seems to only happen if FrameWait is triggered.

@FernetMenta
Copy link
Contributor Author

what exactly do you mean by "if FrameWait is triggered" ?

@smallint
Copy link
Contributor

I mean the event wait:

bool CXBMCRenderManager::FrameWait(int ms)
{
  XbmcThreads::EndTime timeout(ms);
  CSingleLock lock(m_presentlock);
  while(m_presentstep == PRESENT_IDLE && !timeout.IsTimePast())
  {
    printf("FrameWait(%d)\n", timeout.MillisLeft());
    m_presentevent.wait(lock, timeout.MillisLeft());
  }
  return m_presentstep != PRESENT_IDLE;
}

@FernetMenta
Copy link
Contributor Author

the sequence is as follows:

  • FrameWait
  • exit wait when a new frame is signaled by player
  • FrameMove
  • PrepareNextRender, this increments to the next buffer if:
    m_Queue[idx].timestamp <= clocktime + MAXPRESENTDELAY)

so if you see the same buffer rendered twice this condition is not true

could you verify this?

@smallint
Copy link
Contributor

I added an output if !next and this output is not there. Actually flip is only triggered if next is true and the previous log has flips.

-- idle
FrameWait(100)
-- ready
Render(gui=false)
r->r: 34
r: 0  5
-- flip
-- frame
Render(gui=false)
r->r: 6
r: 2  0
-- idle
FrameWait(100)
-- ready
Render(gui=false)
r->r: 41
r: 2  0
-- flip
-- frame
Render(gui=false)
r->r: 1
r: 1  0
-- idle
FrameWait(100)
-- ready
Render(gui=false)
r->r: 35
r: 1  2

Is that Render call between READY and FLIP normal?

@FernetMenta
Copy link
Contributor Author

it is hard for me to follow because I don't see where you placed the logs. it should only render the same buffer twice if the timestamp of the next frame is too far in the future.

Is that Render call between READY and FLIP normal?

not sure what you mean. those are meant to be discrete states which have no "between".
assume we are in the idle state, player delivers a new frame -> we go into ready, FrameMove calls PrepareNextRender which can bring us into FLIP, then PRESENT_FRAME, we render in this state, after render we go back to READY (if there is still a frame in the queue), or IDLE if queue is empty,

@smallint
Copy link
Contributor

OK, I agree that it is hard to follow. I placed the logs where the new state is assigned. I see now that only RenderEx calls g_renderManager.Render() and will try to track that path with more logging. I come back to you when I found something.

@FernetMenta
Copy link
Contributor Author

I think I know the reason. The sequence is wrong. After FrameWait we would need FrameMove to switch to the next buffer.

@FernetMenta
Copy link
Contributor Author

@smallint
Copy link
Contributor

Need to do more tests but yeah, looks good. You are the man!

@smallint
Copy link
Contributor

I could not reproduce the wrong sequence anymore: so we found a bug? ;) Thanks a lot!

@FernetMenta
Copy link
Contributor Author

thanks to you! hopefully this fixes the issue on the Pi too.

@smallint
Copy link
Contributor

I add this patch to my tree and will credit you for it of course. Once I rebase onto baseline it will be removed.

@FernetMenta
Copy link
Contributor Author

jenkins build this please

@FernetMenta
Copy link
Contributor Author

Build error on win unrelated

FernetMenta added a commit that referenced this pull request Jan 23, 2015
enable dirty regions for full screen video
@FernetMenta FernetMenta merged commit 725300f into xbmc:master Jan 23, 2015
@FernetMenta FernetMenta deleted the fullscreen branch January 23, 2015 08:26
@MartijnKaijser MartijnKaijser modified the milestone: I******* 15.0-alpha1 Jan 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants