<html><head></head><body>Hi,<br><br>The implementation ought to allocate enough resources upfront that locking cannot fail. Otherwise, how do you clean up the OpenGL context that you can't even lock??<br><br>AFAICT, the error only exists for repprting invalid parameters, which would be a bug in the app.<br><br><div class="gmail_quote">Le 26 octobre 2019 12:04:43 GMT+01:00, Alexandre Janniaux <ajanni@videolabs.io> 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">Hi,<br><br>On Thu, Oct 24, 2019 at 12:02:13PM +0200, Steve Lhomme wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">On 2019-10-24 11:50, Rémi Denis-Courmont wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #ad7fa8; padding-left: 1ex;">The code is already quite confusing without the patch. It does not make<br>much sense to fail locking the GL, unless there is a bug somewhere else.<br></blockquote>There are 2 calls that can fail OpenglLock() one of them being a call to<br>Apple API. I agree it should not happen in real like, hence the unlikely().<br></blockquote><br>I'm not sure that failing to lock an OpenGL context isn't<br>different than failing to do an allocation. It rarely<br>happens but there are runtime reasons which might lead to<br>this kind of failures too.<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">I think it's good to not silently return no error when in fact the original<br>control was not handled properly. At some point we'll probably have to<br>handle RESET_PICTURES in modules that currently don't handle the controls<br>correctly.<br></blockquote><br>I might not completely understand your point. The previous<br>code was actually returning an error when the control was<br>not handled properly so it alread seemed to match your<br>point. Do I miss something ?<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">In this case we just avoid the code ending up in vlc_assert_unreachable().<br></blockquote><br>I'd say that it might be the correct issue to fix here. If<br>we can fail and that the core can call RESET_PICTURES, we<br>should probably support this in the module instead of<br>silence the error to avoid the assertion, or keep the<br>assertion to highlight that we don't support it.<br><br>Regards,<br>--<br>Alexandre Janniaux<br>Videolabs<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;"> Le 24 octobre 2019 12:14:29 GMT+03:00, Marvin Scholz<br> <epirat07@gmail.com> a écrit :<br><br><br><br> On 24 Oct 2019, at 11:10, Steve Lhomme wrote:<br><br> Otherwise the core will call VOUT_DISPLAY_RESET_PICTURES which we<br> don't support.<br><br> This is what is done in macosx.m when vlc_gl_MakeCurrent() fails.<hr> modules/video_output/caopengllayer.m | 4 ++--<br> 1 file changed, 2 insertions(+), 2 deletions(-)<br><br> diff --git a/modules/video_output/caopengllayer.m<br> b/modules/video_output/caopengllayer.m<br> index 3b542d1e637..249f7b96d5d 100644<br> --- a/modules/video_output/caopengllayer.m<br> +++ b/modules/video_output/caopengllayer.m<br> @@ -343,8 +343,8 @@ static int Control (vout_display_t *vd, int<br> query,<br> va_list ap)<br><br> vout_display_place_t place;<br> vout_display_PlacePicture(&place, &vd->source, &cfg_tmp);<br> - if (OpenglLock(sys->gl))<br> - return VLC_EGENERIC;<br> + if (unlikely(OpenglLock(sys->gl)))<br> + return VLC_SUCCESS;<br><br><br> Can you add a comment here explaining that? Else the code looks quite<br> confusing here without context.<br><br> vout_display_opengl_SetWindowAspectRatio(sys->vgl,<br> (float)place.width / place.height);<br> OpenglUnlock(sys->gl);<br> -- 2.17.1<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><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><br><br><br> --<br> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser<br> ma brièveté.<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><br><br></blockquote><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><br></blockquote><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>