Skip to content
Snippets Groups Projects

video_output: avoid dropping special pictures

Open Steve Lhomme requested to merge robUx4/vlc:vout-special-nodrop into master
2 unresolved threads

Don't drop the first picture and still pictures if they are late. They may be the only pictures available for a while and we need something to display.

Simplify the render_now flag: always true except when we use a new picture that has its own timing (unless it's the first, we want it right away).

This effectively duplicates some of the current usage of the picture_t::b_force flag, which shouldn't have to decide rendering policies.

Merge request reports

Members who can merge are allowed to add commits.

Merge request pipeline #138216 passed

Merge request pipeline passed for 41ff07dc

Approval is optional
Merge blocked: 3 checks failed
Unresolved discussions must be resolved.
Merge conflicts must be resolved.
Merge request must be rebased, because a fast-forward merge is not possible.

Merge details

  • The source branch is 16589 commits behind the target branch.
  • 4 commits will be added to master.
  • Source branch will be deleted.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Steve Lhomme changed milestone to %4.0

    changed milestone to %4.0

    • I am not convinced that we want a special case for the first picture. Showing black before the first picture seems fine to me, and adding a special case could have non-trivial implications when reusing the video output.

    • Author Developer

      There are 2 things at play here

      • dropping the first picture
      • showing the first picture earlier than it should

      IMO the latter shouldn't be there. In practice it's probably rare to have the video ready to show before the audio so it wouldn't make much difference. But if the audio is meant to start "without video" (usually meaning black) then it's wrong.

      For the former it could be a problem on slow decoding scenario. If the first picture arrives late, we could always wait for the next non-late. But there may never be a non-late picture, or not for a while. In this case the VLC approach has been to show at least the first frame and maybe at some point it gets better, rather than showing black until it gets better (ie never).

      So this patch doesn't change this choice. It narrows down the places where changes would be needed to change this. I have some patches after that to narrow this even more (right now the force flag is set on the first decoded picture). In the end that choice should be only in the video output.

    • IMO the latter shouldn't be there. In practice it's probably rare to have the video ready to show before the audio so it wouldn't make much difference. But if the audio is meant to start "without video" (usually meaning black) then it's wrong.

      I'm not sure it's accurate.

      image

      In this sample file, audio PTS is basically earlier than video PTS most of the time, and it's like most of my client streams, and I'm not sure I saw a lot of file where it's not the case. Maybe this could be verified?

      Just for the record, the behaviour "without video" will be platform-dependant. On iOS it will be the application background. On other platforms the previous frame might still be copied on the screen. On video callback we typically don't send a black frame.

    • Author Developer

      I don't see a contradiction between

      probably rare to have the video ready to show before the audio

      and

      audio PTS is basically earlier than video PTS most of the time

      And we're only talking about the first frame here.

    • Indeed, I confused the meaning of the beginning of the sentence, but then I don't understand the meaning of the rest of the sentence:

      IMO the latter shouldn't be there. In practice it's probably rare to have the video ready to show before the audio so it wouldn't make much difference. But if the audio is meant to start "without video" (usually meaning black) then it's wrong.

      Not displaying the first frame earlier will make a difference since video timestamps might be scheduled a bit later than audio, or do I confuse something else?

    • Author Developer

      What is wrong (IMO) is to show the first picture ASAP if it's supposed to start (much) later than the audio. It's not respecting how the file was created in the first place.

    • Well, I'm not sure of how displaying (as UB) the picture from the previous track played in this vout might be more correct either. But without references it seems hard to consider the correct solution.

    • Author Developer

      If the previous track is not cleared when the previous file is unloaded and even present when another file starts. It's a bug. The fact we want to show the first frame only when it's supposed to may exhibit it. That doesn't mean it should not be fixed. And that doesn't mean it should stop us from showing the first frame only when it should.

      In practice the display is restarted between files so I don't think the previous file can remain there. There might be a window without nothing drawn (different behavior per platform). But that's already the case now.

    • Please register or sign in to reply
  • Steve Lhomme mentioned in merge request !641

    mentioned in merge request !641

1509 1510 // next date we need to display again the current picture
1510 1511 date_refresh = sys->displayed.date + VOUT_REDISPLAY_DELAY - render_delay;
1511 1512 refresh = date_refresh <= system_now;
1512 render_now = refresh;
  • Hi,

    It's a bit confusing that render_now is true for picture we might not display.

  • Author Developer

    IMO it's removing some confusion. Right now you have to understand when render_now becomes true or false there.

    When using the "redisplay" mechanism either we want to redisplay in this call
    and refresh is true, we don't need to set render_now to true, as it's the
    default value. When we don't redisplay (refresh = false) we don't need to set
    render_now because we are not going to do the rendering (it can't be the
    first and we didn't drop the current frame in this call). We will return
    VLC_EGENERIC.

    And it probably makes even more sense to always set it to true on redisplay anyway. The PTS of the picture to redisplay is old and we will likely display it right away as there's nothing to wait for. (maybe there's some trick to shift the PTS for redisplay, it doesn't change the fact it's old and can be shown ASAP, waiting between prepare and display is bad)

  • Author Developer

    BTW, when first, refresh and dropped_current_frame are all false, render_now is not used at all. There's some confusion there as well. Should we render now or not at all ? So

    It's a bit confusing that render_now is true for picture we might not display.

    Is a preexisting issue.

  • Please register or sign in to reply
  • added MRStatus::Stale label and removed MRStatus::InReview label

  • added MRStatus::InReview label and removed MRStatus::Stale label

  • Please register or sign in to reply
    Loading