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

[Jan Ekström]

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