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

[Steve Lhomme]

On 2020-10-20 16:20, Romain Vimont wrote:
> 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.

Usually we do vout_fmt = *target for this. But that's often for local 
copies that are known not to use the palette. Maybe a 
video_format_Move() could be added to handle that more gracefully (and 
never have to worry about the palette).

>>> @@ -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.

OK, I missed that part. My idea was to either fully accept or fully 
reject the new format. And create a new display if it doesn't accept the 
new format. Not sure what is better. For example if you fed a 
stereoscopic source to opengl it could decide that it wants a monoscopic 
(?) version instead. Whereas creating a new display might end up picking 
the D3D11 module that could handle it (it can't right now but that's the 
idea).

> For example:
> https://code.videolan.org/videolan/vlc/-/blob/b87ce0d8e30f4b2da7d643c4e2550c22817aeece/modules/video_output/opengl/vout_helper.c#L195-197
> 
> Regards
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>