[FFmpeg-devel] [PATCH 2/2] avformat/tls_schannel: immediately return decrypted data if available

[Jan Ekström]

On Wed, May 13, 2020 at 12:46 PM Jan Ekström <jeebjp at gmail.com> wrote:
>
> On Wed, May 13, 2020 at 3:02 AM Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> >
> > On Wed, May 13, 2020 at 12:12 AM Jan Ekström <jeebjp at gmail.com> wrote:
> > >
> > > Until now, we would have only attempted to utilize already decrypted
> > > data if it was enough to fill the size of buffer requested, that could
> > > very well be up to 32 kilobytes.
> > >
> > > With keep-alive connections this would just lead to recv blocking
> > > until rw_timeout had been reached, as the connection would not be
> > > officially closed after each transfer. This would also lead to a
> > > loop, as such timed out I/O request would just be attempted again.
> > >
> > > By just returning teh available decrypted data, keep-alive based
> > > connectivity such as HLS playback is fixed with schannel.
> > > ---
> > >  libavformat/tls_schannel.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavformat/tls_schannel.c b/libavformat/tls_schannel.c
> > > index 7a8842e7fe..4a1c5901d0 100644
> > > --- a/libavformat/tls_schannel.c
> > > +++ b/libavformat/tls_schannel.c
> > > @@ -392,7 +392,12 @@ static int tls_read(URLContext *h, uint8_t *buf, int len)
> > >      int size, ret;
> > >      int min_enc_buf_size = len + SCHANNEL_FREE_BUFFER_SIZE;
> > >
> > > -    if (len <= c->dec_buf_offset)
> > > +    if (c->dec_buf_offset > 0)
> > > +        /* If we have some left-over data from previous network activity,
> > > +         * return it first in case it is enough. It may contain
> > > +         * data that is required to know whether this connection
> > > +         * is still required or not, esp. in case of HTTP keep-alive
> > > +         * connections. */
> > >          goto cleanup;
> > >
> >
> > Can you move the comment above the if(..)?
> > I feel like if blocks with no braces should have the single statement
> > follow immediately, with no breaks from comments or otherwise, just to
> > make sure its very clear that it belongs together.
> >
> > Change otherwise LGTM.
> >
> > -  Hendrik
>
> Thanks for the reviews.
>
> Yes, I will just move the comment block before the if, that indeed can
> be simpler to view :) .
>
> Will push this patch set with that change and a single "teh" typo in
> the commit message fixed after work in order to give a bit more time
> for comments. In general my test results with HTTPS HLS basically went
> from "unusable" to "zomg!", so I'm quite confident that this is a step
> towards better results with schannel.
>
> Jan

Pushed the set with the two fixes.

Jan