Skip to content
Snippets Groups Projects

Draft: implement resizing through vout_window_t module on iOS

4 unresolved threads

This merge request implements the resizing through the vout_window_t module, removing the need for the OpenGL module to fail on MakeCurrent.

To really make this possible, the rendering is also stopped when the application is put into the background, where no rendering is allowed or wanted anyway.

Resend from https://patches.videolan.org/patch/31498/, including the change on the report function to use booleans, and the change on the lock to use display lock.

Thanks to @umxprime for the multiple investigation and fixes in this patchset!

Draft because I probably need to split the last commit, and integrate more fixes from @umxprime for constraints.

Merge request reports

Members who can merge are allowed to add commits.

Merge request pipeline #162510 passed

Merge request pipeline passed for 9a56b022

Ready to merge by members who can write to the target branch.
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Thomas Guillem
  • Thomas Guillem
  • Thomas Guillem resolved all threads

    resolved all threads

  • Rémi Denis-Courmont
  • 1330 1340 const unsigned frame_rate = todisplay->format.i_frame_rate;
    1331 1341 const unsigned frame_rate_base = todisplay->format.i_frame_rate_base;
    1332 1342
    1333 if (vd->ops->prepare != NULL)
    1334 vd->ops->prepare(vd, todisplay, subpic, system_pts);
    1343 bool rendering_enabled = sys->rendering_enabled &&
    1344 sys->window_width != 0 && sys->window_height != 0;
    1335 1345
    1336 vout_chrono_Stop(&sys->chrono.render);
    1346 if (rendering_enabled)
    1347 {
    1348 if (vd->ops->prepare != NULL)
    1349 vd->ops->prepare(vd, todisplay, subpic, system_pts);
    1350
    1351 vout_chrono_Stop(&sys->chrono.render);
    • The 4.0 vout core should not be messed with except for fixing bugs. We already have vouts doing visibility tracking without involving the core, so I fail to see the justification here, TBH.

      And indeed, it seems rather suspicious that the chronometer is conditionally not stopped, and the size checks look totally gratuitous.

    • And indeed, it seems rather suspicious that the chronometer is conditionally not stopped, and the size checks look totally gratuitous.

      If you stop the chrono, it would register the value, which doesn't mean anything since you didn't do anything that was measurable. If you restart the chrono without stopping, the value is not registered, which is what we want here.

      I can move the size checks to the vout window modules. It was duplicating the checks everywhere but that's not much of an issue for me.

      The 4.0 vout core should not be messed with except for fixing bugs. We already have vouts doing visibility tracking without involving the core, so I fail to see the justification here, TBH.

      This is fixing bugs related to the OpenGL implementation, like the 4.0 version of #26049. I didn't reference them because the code is completely different between 4.0 and 3.0, but the problem remains because it's intrinsic to such feature being present or not, much like !457.

      Alors this is a re-send with the patchset being fixed. I had defer the re-send because I originally did the work again on top of !457 to be sure there is no deadlock that was fixed by the MR and would be harder to track with this code merged on top, but it seems it won't be merged soon without a lot of rework and design.

    • Size checks should be done regardless of visibility state. Trying to render 0x0 frames doesn't make any sense, and is even a protocol error in most display servers.

    • Reporting 0x0 size is currently illegal though.

    • Sure, this is what this MR is changing. Having a zero-size window is still possible though, and even without reporting a 0x0 size, drawing on a 0x0 size is illegal in every renderer API too.

    • I don't think we should allow 0x0. Too much code assumes that it can't happen or that it means some kind of uninitialised state. Cleaning that up seems way beyond the scope of this particular MR.

    • If you're attached to preventing 0x0 reports, I can also handle it through the visibility callback directly from the module itself too.

    • I'm not against 0x0 as such. I'm concerned that allowing it now will expose a lot of latent bugs and we are not at the bestest of times for this kind of endeavour.

    • Please register or sign in to reply
  • Alexandre Janniaux mentioned in merge request !1814 (merged)

    mentioned in merge request !1814 (merged)

  • 661 662 vlc_mutex_unlock(&sys->display_lock);
    662 663 }
    663 664
    665 void vout_ChangeDisplayRenderingEnabled(vout_thread_t *vout, bool enabled)
    666 {
    667 vout_thread_sys_t *sys = VOUT_THREAD_TO_SYS(vout);
    668 assert(!sys->dummy);
    669 vlc_mutex_lock(&sys->display_lock);
    670 sys->rendering_enabled = enabled;
    • As it stands, an atomic can be used

    • Not using mutex means that the window system will not wait for rendering to stop when setting visibility to false.

    • That's not in line with existing uses of visibility, which is purely advisory and non-blocking, and makes it a poor substitute for those.

    • I don't undestand what you are saying, where is visibility currently reported, and what is the purpose of advisory and non-blocking visibility reporting?

    • The purpose of existing visibility checks is to skip unnecessary rendering and save CPU cycles in the display server. It would be better if it could also skip display converters, but that could just as well be done with an atomic value.

      But an optimisation can't afford to block the window event loop waiting on the display lock. Thus, unless people agree to fix the display prepare-display coupling and to require display callbacks complete fast, we can't take the display lock here to update visibility. In other words, at least in 4.0, this can't take the display lock.

    • And this MR does not even seem to skip the display converters. If so, involving the core is pretty useless.

    • The purpose of existing visibility checks is to skip unnecessary rendering and save CPU cycles in the display server. It would be better if it could also skip display converters, but that could just as well be done with an atomic value.

      Which existing visibility checks?

      Thus, unless people agree to fix the display prepare-display coupling and to require display callbacks complete fast, we can't take the display lock here to update visibility.

      I think we already proved Videolabs wants to fix the issues with the current video output state, but as re-explained during the vout workshop by Romain, I failed to see how splitting the prepare-display coupling changes anything.

      Also, back at visibility, even though you only consider visibility as an optimization and the time when rendering is being disabled doesn't matter, you cannot make the same argument for when visibility is restored and you want to restore rendering, since you are creating glitches otherwise. This is litterally the same argument as yours regarding resizing. Visibility is as rendering state much like size and must be passed synchronously to the rendering.

      What's missing in this merge request to completely achieve this still depends on how the display interface is designed for 4.0 and 5.0, ie. depending on whether the rendering state must be completely driven by the vout or how much it is deported to the display. Likely, if we want asynchronous displays, we'd also want a control to signal the display too.

      What's beneficial in implementing this in the core is that it's handled by default regardless of the displays, it's usable by the video output state to know what needs to be sampled (and thus, useful for 5.0 and VSYNC handling) and it can be forwarded to converters and filters (which is not done currently because the filter part doesn't exist for that, the converter is not in a separate chain and the filter is not able to report the history it needs). Those changes are obviously not in this merge request because it's too big, but also because I haven't done them and I cannot fix everything at once just to fix the one bug I care about right now.

      In other words, at least in 4.0, this can't take the display lock.

      4.0 needs mandatory fixing for this issue, the windowing situation is horribly broken compared to 3.0, and features like 360 are unusable. But regardless of that, the merge request is made to solve existing class of bugs and failures on systems able to provide backgrounding. If it's not implemented for other platform, then it avoids the long-waiting issue anyway, and on those where it must be implemented, it fixes crashes and MakeCurrent() failures, so where the waiting/signalling was necessary anyway.

    • I disagree that windowing is broken and I refuse to see major changes to the plugin interface there any more in 4.0. 360 was always half-baked in 3.0 and that's already been pointed out including by me. That's got nothing to do with this.

      As I've noted in the past, I do think adding visibility as a window property makes sense but it's rather late in 4.0 to be changing all the affected window providers and displays. In any case, in this current shape it can't even handle the ppre-existing visibility management in displays.

      -1

    • I disagree that windowing is broken and I refuse to see major changes to the plugin interface there any more in 4.0. 360 was always half-baked in 3.0 and that's already been pointed out including by me. That's got nothing to do with this.

      We've been talking about that for years and we even did two workshop about this. Resizing has become completely glitchy, and it's not tearing but sometimes seconds before the new size is rendered. Don't get me wrong: the architectural changes are better in the master version than in 3.0, but I can't just grasp how you can disagree that windowing is broken while being part of those two workshops.

      360 was always half-baked in 3.0 and that's already been pointed out including by me. That's got nothing to do with this.

      Well, I was fixing it the past three years, but now I can't even test what I'm fixing.

      As I've noted in the past, I do think adding visibility as a window property makes sense but it's rather late in 4.0 to be changing all the affected window providers and displays.

      As mentioned, we don't need to implement it to more than the display where it creates fatal errors.

      In any case, in this current shape it can't even handle the ppre-existing visibility management in displays.

      Why? It seems to me that it's completely independent. Existing display (which ones again??) can still handle visibility the way they want, it's just merely duplicating what the window is doing until we create a control for that.

    • we don't need to implement it to more than the display where it creates fatal errors.

      If it's macOS-specific, it shouldn't be part of the window core API in the first place.

      And visibility cannot create fatal errors in the first place.

      If you mean mandatory preemption, that's not visibility, and I don't think it can be handled at the window/display interface, as it typically means not only the window is invisible but the hardware buffers are released.

      Existing display (which ones again??) can still handle visibility the way they want, it's just merely duplicating what the window is doing until we create a control for that.

      Just like X11 handled it internally until 35dd78c3, macOS can handle it internally. The only interest the core could conceivably have in visibility is to skip unnecessary converters in the core display code.

      This MR doesn't seem to do that, and it's questionable if it's worth adding in 4.0 anyway. And then it couldn't afford to block the window event loop, so it can't take the display lock in its current slow state.

      Edited by Rémi Denis-Courmont
    • If you mean mandatory preemption, that's not visibility, and I don't think it can be handled at the window/display interface, as it typically means not only the window is invisible but the hardware buffers are released.

      I have it working for a year on iOS, which is probably the most restrictive OS regarding that. Resources preemption would be nice but that's not mandatory here.

      Just like X11 handled it internally until 35dd78c3, macOS can handle it internally. The only interest the core could conceivably have in visibility is to skip unnecessary converters in the core display code.

      Sure it can with !1316 but then you cannot assert that vlc_gl_MakeCurrent cannot fail in the current design.

      Also, the same problem arises with filters anyway, and there's no more solution to the problem currently for those.

    • You can't skip filters, only converters before the display. Specifically filters can be stateful (deinterlacer) or have side effects (scene), so they have to keep processing even if the window is invisible. And besides, filters may run in a transcode chain without a window at all.

      If you need to stop filters, you need to deselect the track or stop the input. That's necessarily at a much higher level of handling than this patch proposes.

      Similarly, you can't rely on this MR to deal with GL context errors.

    • You can't skip filters, only converters before the display. Specifically filters can be stateful (deinterlacer) or have side effects (scene), so they have to keep processing even if the window is invisible. And besides, filters may run in a transcode chain without a window at all.

      I know, I've mentioned that too with potential solutions. :smile:

      If you need to stop filters, you need to deselect the track or stop the input. That's necessarily at a much higher level of handling than this patch proposes.

      That's the only possible way to handle this currently, but it's worse than pause or dropping the pictures for the filters and notifying them just like we'd signal a discontinuity at the stream level.

    • I don't really see how it's worse. If you lost the video hardware to some other process, to a VPU/GPU reset or to a lower power state, you've probably lost the reference pictures of the video decoder, the shaders and what-not.

      At any rate, I don't think a design of visibility that is so specific to one single platform and unsuitable on others, and doesn't even solve the underlying problem except in all cases should be added to the core. Especially not when the core does essentially nothing with it that the display could not do.

    • Please register or sign in to reply
    • If it's macOS-specific, it shouldn't be part of the window core API in the first place.

      At any rate, I don't think a design of visibility that is so specific to one single platform and unsuitable on others, and doesn't even solve the underlying problem except in all cases should be added to the core. Especially not when the core does essentially nothing with it that the display could not do.

      Note also that even though the fatal error are happening only on MacOS, it also results in a lot of warnings and picture stalling when another window covers the VLC vout window on wayland, which is making the whole "too late" message feedback completely useless and likely to make the users create issues with this:

      [0000616000088880] main vout display debug: picture displayed late (missing 968 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 967 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 934 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 901 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 867 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 834 ms)
      [000061900005resizeff80] main video output warning: picture is too late to be displayed (missing 801 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 767 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 734 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 701 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 667 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 634 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 601 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 567 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 534 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 501 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 467 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 434 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 401 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 367 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 334 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 301 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 267 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 234 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 201 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 168 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 134 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 101 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 68 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 35 ms)
      [0000616000088880] main vout display debug: picture displayed late (missing 969 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 999 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 966 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 933 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 899 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 866 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 833 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 799 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 766 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 733 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 699 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 666 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 633 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 599 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 566 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 533 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 499 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 466 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 433 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 400 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 366 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 333 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 300 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 266 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 233 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 200 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 166 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 133 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 100 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 67 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 35 ms)
      [0000616000088880] main vout display debug: picture displayed late (missing 937 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 998 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 965 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 931 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 898 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 865 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 831 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 798 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 765 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 731 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 698 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 665 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 631 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 598 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 565 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 531 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 498 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 465 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 431 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 398 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 365 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 331 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 298 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 265 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 231 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 198 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 1195 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 1162 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 1128 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 1095 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 1062 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 1028 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 995 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 962 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 928 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 895 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 862 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 828 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 795 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 762 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 728 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 695 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 662 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 628 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 595 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 562 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 529 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 495 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 462 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 429 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 395 ms)
      [000061900005ff80] main video output warning: picture is too late to be displayed (missing 362 ms)
    • But that's the point: visibility for Wayland (or X11) displays is a performance optimisation. We can't block the window event loop waiting on the currently slow display lock for the sake of a performance optimisation. That's why I'm arguing that it should be an atomic.

      If/when the vout display interface is fixed, then we can take the display lock, and while at it, add the missing notification callback to the display module. But I can't imagine this happening in 4.0.

    • If/when the vout display interface is fixed, then we can take the display lock, and while at it, add the missing notification callback to the display module. But I can't imagine this happening in 4.0.

      During the resize fixes on !1820 (merged) , I had the intermediate state where the window is giving DISPLAY_SIZE event and the vout display still following them. You approximately have 2 event per second taken into account because of the current vout loop, which means that the view is resized to the correct size at first, then come back to the old size, then sloooowly resize to the new size. Though the resize doesn't showcase this after the display changes, it's still blocking the vout window event loop each time it is waiting to acquire the display lock. I cannot imagine not fixing this for 4.0.

    • I cannot accept an implementation of visibility that blocks on the slow/4.0 version of the display lock (due to strict prepare-display pairing). I'm not spending so much effort fixing mouse events for somebody to knowingly re-add the same problem with visibility.

    • How is that different from resize and how is that like mouse event?

    • What is that supposed to mean? There are work-arounds proposed for both resize and mouse events. Meanwhile this MR introduces a new occurrence of the problem.

    • Mouse events can and should be processed with the state that was visible to the user, so it actually doesn't need display lock. Admittedly, it could be argued that the visible state is racy for the user anyway, and that it might not be the state that he expects too, but it's also much easier to handle that way so there's no real reason for having the problem here.

      It's a problem alike resize, indeed, but then it would not be a problem as soon as it's fixed for resize?

    • Mouse events can and should be processed with the state that was visible to the user, so it actually doesn't need display lock.

      Yes, we already agreed on that point (and disagreed with others) in other MR. But currently and until I complete my task, the fact of the matter remains that mouse events require the display lock on the window event loop.

      I don't want new problems with the display lock. Ideally, visibility would take the display to send an optional notification to the display, but I can't see that working well in 4.0.

      Edited by Rémi Denis-Courmont
    • Please register or sign in to reply
  • mentioned in issue VLCKit#682

  • Alexandre Janniaux mentioned in merge request !6446

    mentioned in merge request !6446

  • Please register or sign in to reply
    Loading