<html><head></head><body>No and no.<br><br><div class="gmail_quote">Le 11 février 2021 09:31:33 GMT+02:00, Steve Lhomme <robux4@ycbcr.xyz> a écrit :<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<pre class="k9mail">On 2021-02-10 16:53, Rémi Denis-Courmont wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">Le keskiviikkona 10. helmikuuta 2021, 8.38.11 EET Steve Lhomme a écrit :<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #ad7fa8; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"> @@ -543,7 +546,7 @@ void vout_window_Disable(vout_window_t *window);<br><br> static inline void vout_window_ReportSize(vout_window_t *window,<br> <br> unsigned width, unsigned<br> height)<br> <br> {<br><br> - window->owner.cbs->resized(window, width, height);<br> + window->owner.cbs->resized(window, width, height, NULL, NULL);<br><br> }<br></blockquote>IMO this function should always expose the new parameter or have a new<br>variant with the extra params. Just calling the callback manually (as<br>done in 2/2) defeats the purpose of having a helper in the first place.<br></blockquote>The purpose of the helper is to help, which is what it does, including filling<br>default parameters.<br><br>Providing default values for rarely used parameters is possible, but it's<br>really ugly in pure C (as opposed to C++).<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #ad7fa8; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"> /**<br><br> diff --git a/src/video_output/video_output.c<br> b/src/video_output/video_output.c index d04d3f4d11..fb7f54e385 100644<br> --- a/src/video_output/video_output.c<br> +++ b/src/video_output/video_output.c<br> @@ -543,7 +543,8 @@ void vout_ChangeWindowState(vout_thread_t *vout,<br> unsigned st)><br> }<br> <br> void vout_ChangeDisplaySize(vout_thread_t *vout,<br><br> - unsigned width, unsigned height)<br> + unsigned width, unsigned height,<br> + void (*cb)(void *), void *opaque)<br></blockquote>There's no guarantee this callback signature will match<br>vout_window_ack_cb if it changes.<br></blockquote>It does not match in the first place.<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #ad7fa8; padding-left: 1ex;">There should be a cast to that type.<br></blockquote>What? You can't cast function types. That is UB in theory, and breaks CFI in<br>practice.<br></blockquote><br>Similar to what we do with some module callback:<br> vlc_decoder_device_Open open__ = activate;<br> (void) open__;<br> set_callback(activate)<br><br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #ad7fa8; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"> {<br> <br> vout_thread_sys_t *sys = VOUT_THREAD_TO_SYS(vout);<br><br> diff --git a/src/video_output/window.c b/src/video_output/window.c<br> index 9c2df5c1e6..c866fe7af4 100644<br> --- a/src/video_output/window.c<br> +++ b/src/video_output/window.c<br> @@ -214,13 +228,16 @@ typedef struct vout_display_window<br><br> } vout_display_window_t;<br> <br> static void vout_display_window_ResizeNotify(vout_window_t *window,<br><br> - unsigned width, unsigned<br> height) + unsigned width,<br> unsigned height, +<br> vout_window_ack_cb cb, + void<br> *opaque)<br><br> {<br> <br> vout_display_window_t *state = window->owner.sys;<br> vout_thread_t *vout = state->vout;<br><br> + struct vout_window_ack_data data = { window, cb, opaque };<br><br> msg_Dbg(window, "resized to %ux%u", width, height);<br><br> - vout_ChangeDisplaySize(vout, width, height);<br> + vout_ChangeDisplaySize(vout, width, height, vout_window_Ack, &data);<br></blockquote>Using a local stack structure forces vout_ChangeDisplaySize() to be<br>synchronous.<br></blockquote>The whole point of the callback is that it's synchronous.<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #ad7fa8; padding-left: 1ex;">Which is kind of contradictory to using a callback that<br>look asynchronous (but is not).<br></blockquote>No, you're contradictory. The callback *is* synchronous therefore it *can* use<br>stack objects. (Like many here) I prefer not to call xmalloc() if I don't have<br>to.<br></blockquote><br>My point is that reading this code there is zero indication that it's <br>synchronous. Not in the type, not in the name, not in the documentation. <br>Passing a callback to a function is usually a hint that it's <br>asynchronous. Otherwise you'd just get a return value.<hr>vlc-devel mailing list<br>To unsubscribe or modify your subscription options:<br><a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a></pre></blockquote></div><br>-- <br>Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.</body></html>