[vlc-devel] [PATCH 09/39] vout: request the display with a decoder device

[Alexandre Janniaux]

Hi,

They are numerous points of reflexion that are coming with all
you mentionned, I'll try to split my post, but would suggest to
split the discussion instead.

On Sat, Oct 05, 2019 at 12:21:51PM +0300, Rémi Denis-Courmont wrote:
> Le perjantaina 4. lokakuuta 2019, 12.47.01 EEST Alexandre Janniaux a écrit :
> > Hi,
> >
> > That's indeed a pivotal point to tackle, but it's probably not an
> > issue in this design.
> >
> > The main point of window providing is that you don't have to actually
> > close the native window when the vout_window is closed by the core.
> >
> > Audio visualization are creating a vout_thread, which creates a window
> > so this doesn't conflict with this AFAIK.
>
> An audio visualisation may create a video output as a helper to output
> bitmaps, but that is an implementation detail of the audio visualisation plug-
> in and indeed some (GL-based) don't create a video output already now.

That's indeed an implementation detail, and I believe you're talking
about GLSpectrum?

Visualization could work that way but for the case of GLSpectrum, I
think this is actually an uncomplete example, as the module is using
OpenGL without the linking abstraction and recreating its own video
synchronization. It is more like an experiment than a real example
of what should be done imho. But that's more a sidenote than an
counter argument.

However, you're right on the fact that an audio visualization plugin
could use their own window for different kind of result. In this
situation there are two cases:
+ either we don't care about window providing, the core will do it
  and there are no issues as it is the same situation as currently.
+ or we would like the window to be the one from the interface if
  possible, meaning in any case that the window provider is known.

Which brings us to your second point:

> Obviously if the window or the window allocator would have to be supplied to
> both the audio side for visualisation and the video side for video output. But
> the window can be (and already sometimes is) used without a video output. So
> it seems unavoidable that the video output is created from the window and not
> vice-versa.

(side note:
 window allocator is probably a better name to avoid conflicts between
 the existing window provider definition for modules and the one we're
 trying to introduce as core window provider, but I'm not sure it won't
 be confusing to have both. Maybe they should all be changed?)

If the window provider is called but the native window surface is
still in use, it could either:
+ provide a surface and an interface feedback for the user, like
  a tab, a split, a PiP, etc.
+ close the other user of its surface.
+ return an error to signal it can't provide a window, and it
  fallback to another window providing method.

IIUC, your point is that with the current design for vout_thread,
which is the one we agreed on during the window vout workshop, we would
have issues recycling the window for visualization after it has been
used by any video output thread, because the vout thread won't be killed.

Also IIUC, your solution is to have the window be created out of the
vout thread, and have the vout thread killed because you used another
client of the window, or because the video ended ?

To me, this seems quite in contradiction with what was applied on the
source code related to vout lifecycle. Thomas can probably make the
situation clearer, but to me the vout thread was to be recycled and kept,
and that we could draw a parallel between vout_thread and aout_thread.

Also, I don't really understand why this model won't work. As above,
I mentionned the interface could kill its user (the vout thread) if
needed, so it's more a matter of signalling current state, which I
agree is still difficult but won't circumvent the design as it is as
hard to define as the other issues that you have with a GetWindow /
SetWindow oriented implementation). The issue you're mentionning is
really not about keeping the window and killing the vout but instead
killing the users of the window at all, because as I mentionned in a
previous mail, native window surfaces can be reused with the vout_window
object being destructed. The drawable vouts are exactly that, and my
subsurface capability, albeit imperfect, is really to be a more
encapsulated approach of that. I planned on adding a vout_window container
for subsurface to solve my current issues, much like the initial vout
window container module suggested by Thomas, but unfortunately my free
time got busy.

My implementation is missing some point to be feature-complete, but has
quite many advantages in comparison with a GetWindow() approach (that I
did right before dropping it for the current). For example, in the case
of mosaic, the "mosaic" splitter could almost litteraly just be a window
provider for different vout thread. In term of UI interface, it means
that an interface provider (for instance an interface module implementation)
can provide as many video window as it wants.

As it seems that we failed reaching consensus at first try, maybe we
can widen the discussion to the whole design in a new ML thread and
avoid polluting this review. I'll try to provide a summary of the last
mails but you might provide a standalone answer in a dedicated thread
if you would like to. In any case, it's a bit hard for me to mix
different difficult subjects in one review of a difficult patchset so
I would appreciate that. I just wanted to highlight that it seemed to
work without changing the current design of vouts.

> Back to the decoder device, it comes down to maximizing commonality between
> the different decoder use cases. As far as I can guess, there's more
> opportunity for sharing code if the decoder device comes from the window to
> the decoder, and then *from* the decoder to the video output rather than from
> the video output to the decoder. Simply because we will eventually have
> decoder device without video output.

TBH, I don't follow that point at all in the light of my current
understanding of last reviews of Steve work and his explanation.
I might be missing some of the changes during the last reviews.

I guess that you're talking about transcode use case?

As far as it went last time I tried to summarize decoder device
lifecycle and usage, the first client of the decoder device is
the decoder itself -- you seem to agree with this if I'm not wrong.

For the other client, it could be either a video output or an
encoder. The case of the video output is the one covered by this
patch(set). In particular, it exists outside of the displat-related
vout_thread code and is conveyed by the creator of the vout_thread
so it's already crafted to handle a lot of different use case and
close to an video-output independant code.

The case of transcoding won't be that different, you get it from
somewhere, which is probably priority or parameter, and then you
create a decoder passing the decoder device, and the encoder
passing the same decoder device.

I'd guess that the code you'd like to share is the one you would
write in the decoder_UpdateVideoFormat, given how you highlighted
"from the decoder to the video output", or maybe decoder_QueueVideo.
I don't really see what kind of code you plan to put in common
given the already well-defined role of decoder device, decoder,
transcode_encoder and video_output, and would be curious of what
you suggest.

Also, push is an enormeous work. I'm all for merging everything
that can at least make it work and is reasonable to merge, then
tweak and simplify the implementation, rather than playing dice
on the shape of the future unrelated patches.
Steve has made code cleanup for the transcoding pipeline with
decoder device and push in mind, so I'm pretty sure he already
handle the issues of code reuse better than we can do in a
partial review.

This patch looks clearly adequate for the current design to me
and seems to be going to the design that I understood from Steve,
which I find quite simple and reasonable for all our current use
case and the ones I'm working on.

Regards,
--
Alexandre Janniaux
Videolabs