diff mbox series

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

Message ID 20200512220525.9911-3-jeebjp@gmail.com
State Accepted
Headers show
Series avformat/tls_channel: fixes to make keep-alive work
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Jan Ekström May 12, 2020, 10:05 p.m. UTC
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(-)

Comments

Hendrik Leppkes May 12, 2020, 10:15 p.m. UTC | #1
On Wed, May 13, 2020 at 12:12 AM Jan Ekström <jeebjp@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
Jan Ekström May 13, 2020, 9:46 a.m. UTC | #2
On Wed, May 13, 2020 at 3:02 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
> On Wed, May 13, 2020 at 12:12 AM Jan Ekström <jeebjp@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
Jan Ekström May 13, 2020, 2:21 p.m. UTC | #3
On Wed, May 13, 2020 at 12:46 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Wed, May 13, 2020 at 3:02 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> >
> > On Wed, May 13, 2020 at 12:12 AM Jan Ekström <jeebjp@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
diff mbox series

Patch

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;
 
     if (c->sspi_close_notify)