[vlc-devel] [PATCH 1/5] vout: update the format after filters

[Romain Vimont]

On Tue, Oct 20, 2020 at 03:59:27PM +0200, Steve Lhomme wrote:
> HI,
> 
> On 2020-10-20 14:32, Romain Vimont wrote:
> > diff --git a/src/video_output/video_output.c b/src/video_output/video_output.c
> > index 57ee54e6e8..15c1f0e3ee 100644
> > --- a/src/video_output/video_output.c
> > +++ b/src/video_output/video_output.c
> > @@ -950,6 +950,34 @@ typedef struct {
> >       config_chain_t *cfg;
> >   } vout_filter_t;
> > +static int
> > +ThreadRequestVoutFormatChange(vout_thread_sys_t *sys, const video_format_t *fmt,
> > +                              vlc_video_context *vctx, video_format_t *target)
> > +{
> > +    vout_display_t *vd = sys->display;
> > +
> > +    video_format_t vout_fmt;
> > +    int ret = video_format_Copy(&vout_fmt, fmt);
> > +    if (ret != VLC_SUCCESS)
> > +        return ret;
> > +
> > +    vlc_mutex_lock(&sys->display_lock);
> > +    ret = VoutUpdateFormat(vd, &vout_fmt, vctx);
> > +    vlc_mutex_unlock(&sys->display_lock);
> > +
> > +    if (ret != VLC_SUCCESS)
> > +    {
> > +        video_format_Clean(&vout_fmt);
> > +        return ret;
> > +    }
> > +
> > +    /* Move vout_fmt to *target */
> > +    video_format_Clean(target);
> > +    memcpy(target, &vout_fmt, sizeof(*target));
> 
> You should use video_format_Copy()

No, it's a "move" (in the Rust/C++ sense): target directly takes
ownership of the inner data of vout_fmt (and vout_fmt content is
considered undefined).

This avoids an unnecessary deep-copy followed by the destruction of the
source.

> > @@ -1043,13 +1071,28 @@ static void ThreadChangeFilters(vout_thread_sys_t *vout)
> >       }
> >       if (!es_format_IsSimilar(p_fmt_current, &fmt_target)) {
> > -        msg_Dbg(&vout->obj, "Adding a filter to compensate for format changes");
> > -        if (filter_chain_AppendConverter(sys->filter.chain_interactive,
> > -                                         &fmt_target) != 0) {
> > -            msg_Err(&vout->obj, "Failed to compensate for the format changes, removing all filters");
> > -            ThreadDelAllFilterCallbacks(vout);
> > -            filter_chain_Reset(sys->filter.chain_static,      &fmt_target, vctx_target, &fmt_target);
> > -            filter_chain_Reset(sys->filter.chain_interactive, &fmt_target, vctx_target, &fmt_target);
> > +        msg_Dbg(&vout->obj, "Changing vout format to %4.4s",
> > +                            (const char *) &p_fmt_current->video.i_chroma);
> > +
> > +        int ret = ThreadRequestVoutFormatChange(vout, &p_fmt_current->video,
> > +                                                vctx_current,
> > +                                                &fmt_target.video);
> > +        if (ret == VLC_SUCCESS)
> > +            fmt_target.i_codec = fmt_target.video.i_chroma;
> 
> This could be done in ThreadRequestVoutFormatChange()

ThreadRequestVoutformatchange() only takes the video_format_t as
parameter, not the full es_format_t. We could change its signature so
that it could also do this assignment, but I'm not sure it's better.

> 
> > +        else
> > +            msg_Dbg(&vout->obj, "Changing vout format to %4.4s failed",
> > +                                (const char *) &p_fmt_current->video.i_chroma);
> > +
> > +        if (!es_format_IsSimilar(p_fmt_current, &fmt_target))
> 
> This test is probably not needed, you can put the code in the else above. If
> you really want to double check, just add an assert().

On vout format update, the vout may request some changes from the core
(the same way as during the Open()), so fmt_target may have changed
here.

For example:
https://code.videolan.org/videolan/vlc/-/blob/b87ce0d8e30f4b2da7d643c4e2550c22817aeece/modules/video_output/opengl/vout_helper.c#L195-197

Regards