Thanks Michael for review. I updated the iteration to address your feedback: https://patchwork.ffmpeg.org/patch/12540/
Best Regards, -Jun On Sat, Mar 30, 2019 at 6:14 PM Michael Niedermayer <mich...@niedermayer.cc> wrote: > On Fri, Mar 29, 2019 at 03:59:21PM -0700, Jun Li wrote: > > On Wed, Mar 27, 2019 at 11:27 PM Jun Li <junli1...@gmail.com> wrote: > > > > > > > > > > > On Mon, Mar 25, 2019 at 2:42 PM Jun Li <junli1...@gmail.com> wrote: > > > > > >> > > >> > > >> On Fri, Mar 22, 2019 at 4:12 PM Jun Li <junli1...@gmail.com> wrote: > > >> > > >>> The current setting for send-100-continue option is either > > >>> enabled if applicable or forced enabled, no option to force > > >>> disable the header. This change is to expand the option setting > > >>> to provide more flexibility, which is useful for rstp case. > > >>> --- > > >>> libavformat/http.c | 28 +++++++++++++++++----------- > > >>> libavformat/icecast.c | 2 +- > > >>> libavformat/rtsp.c | 1 + > > >>> 3 files changed, 19 insertions(+), 12 deletions(-) > > >>> > > >>> diff --git a/libavformat/http.c b/libavformat/http.c > > >>> index 74d743850d..5a937994cf 100644 > > >>> --- a/libavformat/http.c > > >>> +++ b/libavformat/http.c > > >>> @@ -113,6 +113,7 @@ typedef struct HTTPContext { > > >>> uint8_t *inflate_buffer; > > >>> #endif /* CONFIG_ZLIB */ > > >>> AVDictionary *chained_options; > > >>> + /* -1 = try to send if applicable, 0 = always disabled, 1 = > always > > >>> enabled */ > > >>> int send_expect_100; > > >>> char *method; > > >>> int reconnect; > > >>> @@ -155,7 +156,7 @@ static const AVOption options[] = { > > >>> { "auth_type", "HTTP authentication type", > > >>> OFFSET(auth_state.auth_type), AV_OPT_TYPE_INT, { .i64 = > HTTP_AUTH_NONE }, > > >>> HTTP_AUTH_NONE, HTTP_AUTH_BASIC, D | E, "auth_type"}, > > >>> { "none", "No auth method set, autodetect", 0, > AV_OPT_TYPE_CONST, { > > >>> .i64 = HTTP_AUTH_NONE }, 0, 0, D | E, "auth_type"}, > > >>> { "basic", "HTTP basic authentication", 0, AV_OPT_TYPE_CONST, { > > >>> .i64 = HTTP_AUTH_BASIC }, 0, 0, D | E, "auth_type"}, > > >>> - { "send_expect_100", "Force sending an Expect: 100-continue > header > > >>> for POST", OFFSET(send_expect_100), AV_OPT_TYPE_BOOL, { .i64 = 0 }, > 0, 1, E > > >>> }, > > >>> + { "send_expect_100", "Force sending an Expect: 100-continue > header > > >>> for POST", OFFSET(send_expect_100), AV_OPT_TYPE_BOOL, { .i64 = -1 }, > -1, 1, > > >>> E }, > > >>> { "location", "The actual location of the data received", > > >>> OFFSET(location), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D | E }, > > >>> { "offset", "initial byte offset", OFFSET(off), > AV_OPT_TYPE_INT64, > > >>> { .i64 = 0 }, 0, INT64_MAX, D }, > > >>> { "end_offset", "try to limit the request to bytes preceding > this > > >>> offset", OFFSET(end_off), AV_OPT_TYPE_INT64, { .i64 = 0 }, 0, > INT64_MAX, D > > >>> }, > > >>> @@ -1179,16 +1180,21 @@ static int http_connect(URLContext *h, const > > >>> char *path, const char *local_path, > > >>> local_path, method); > > >>> proxyauthstr = > ff_http_auth_create_response(&s->proxy_auth_state, > > >>> proxyauth, > > >>> local_path, method); > > >>> - if (post && !s->post_data) { > > >>> - send_expect_100 = s->send_expect_100; > > >>> - /* The user has supplied authentication but we don't know > the > > >>> auth type, > > >>> - * send Expect: 100-continue to get the 401 response > including > > >>> the > > >>> - * WWW-Authenticate header, or an 100 continue if no auth > > >>> actually > > >>> - * is needed. */ > > >>> - if (auth && *auth && > > >>> - s->auth_state.auth_type == HTTP_AUTH_NONE && > > >>> - s->http_code != 401) > > >>> - send_expect_100 = 1; > > >>> + > > >>> + if (post && !s->post_data) { > > >>> + if (s->send_expect_100 != -1) { > > >>> + send_expect_100 = s->send_expect_100; > > >>> + } else { > > >>> + send_expect_100 = 0; > > >>> + /* The user has supplied authentication but we don't > know > > >>> the auth type, > > >>> + * send Expect: 100-continue to get the 401 response > > >>> including the > > >>> + * WWW-Authenticate header, or an 100 continue if no > auth > > >>> actually > > >>> + * is needed. */ > > >>> + if (auth && *auth && > > >>> + s->auth_state.auth_type == HTTP_AUTH_NONE && > > >>> + s->http_code != 401) > > >>> + send_expect_100 = 1; > > >>> + } > > >>> } > > >>> > > >>> #if FF_API_HTTP_USER_AGENT > > >>> diff --git a/libavformat/icecast.c b/libavformat/icecast.c > > >>> index c93b06b553..d2198b78ec 100644 > > >>> --- a/libavformat/icecast.c > > >>> +++ b/libavformat/icecast.c > > >>> @@ -115,7 +115,7 @@ static int icecast_open(URLContext *h, const char > > >>> *uri, int flags) > > >>> av_dict_set(&opt_dict, "auth_type", "basic", 0); > > >>> av_dict_set(&opt_dict, "headers", headers, 0); > > >>> av_dict_set(&opt_dict, "chunked_post", "0", 0); > > >>> - av_dict_set(&opt_dict, "send_expect_100", s->legacy_icecast ? > "0" : > > >>> "1", 0); > > >>> + av_dict_set(&opt_dict, "send_expect_100", s->legacy_icecast ? > "-1" > > >>> : "1", 0); > > >>> if (NOT_EMPTY(s->content_type)) > > >>> av_dict_set(&opt_dict, "content_type", s->content_type, 0); > > >>> else > > >>> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c > > >>> index ae8811234a..6e67e411b9 100644 > > >>> --- a/libavformat/rtsp.c > > >>> +++ b/libavformat/rtsp.c > > >>> @@ -1793,6 +1793,7 @@ redirect: > > >>> sessioncookie); > > >>> av_opt_set(rt->rtsp_hd_out->priv_data, "headers", headers, > 0); > > >>> av_opt_set(rt->rtsp_hd_out->priv_data, "chunked_post", "0", > 0); > > >>> + av_opt_set(rt->rtsp_hd_out->priv_data, "send_expect_100", > "0", > > >>> 0); > > >>> > > >>> /* Initialize the authentication state for the POST session. > > >>> The HTTP > > >>> * protocol implementation doesn't properly handle > multi-pass > > >>> -- > > >>> 2.17.1 > > >>> > > >> > > >> > > >> Ping. > > >> The current setting for send_100_expect in http.c is applicable only > when > > >> request is POST > > >> and post body is empty. The code checks the post body as this: > > >> * + if (post && !s->post_data) { * > > >> ...... > > >> > > >> This is absolutely correct. But rtsp tunnel in http is a special > case. It > > >> post the http header > > >> without any post_data, and then write the tunneled data into the tcp > file > > >> descriptor -- that is > > >> two steps and skips the s->post_data. > > >> > > >> So in the case, it exactly falls into the send_100_expect condition > > >> although the real post data > > >> will be written to fd later. It is also an issue when I test it with > IP > > >> cameras because they don't > > >> support "Expect: 100-continue". > > >> > > >> I don't see a smart fix in http.c because whoever use http.c can skip > the > > >> http API and write > > >> to the TCP fd directly. This kind of behavior will create a a valid > post > > >> operation but http.c > > >> would never get any information of post_data. It kind of skips the > http > > >> layer and directly talk > > >> to the tcp. > > >> > > >> So the patch here I proposed another path --- trust the user who is > using > > >> http.c. In this specific > > >> case, it is rtsp.c who is using http.c, so it is rtsp's obligation to > > >> configure the correct header since > > >> it choose to skip http and write post data directly to the file > > >> descriptor. > > >> > > >> This is how this patch comes out. > > >> > > >> Thanks > > >> -Jun > > >> > > > > > > > > > Ping x 2 > > > Someone met the same issue with me, ticket 7297 -- > > > https://trac.ffmpeg.org/ticket/7297 > > > > > > This patch is to fix the rtsp tunnel http issue. The root cause is the > > > "Expect:100-continue" in the http header, it is applicable only when > > > request is POST and post body is empty. The code checks the post body > is as > > > following in http.c: > > > * + if (post && !s->post_data) { * > > > ...... > > > > > > This is correct in most cases but rtsp tunnel in http is special. It > post > > > the http header WITHOUT any post_data, and then write the post data > into > > > the tcp file descriptor directly. > > > > > > So in the case, it exactly falls into the send_100_expect condition > > > although the real post data will be written to fd later. It is also an > > > issue when I test it with IP cameras because they don't support > "Expect: > > > 100-continue". > > > > > > So this patch is for fixing the rtsp tunnel http issue. > > > > > > Thanks > > > -Jun > > > > > > > > > Ping x 3. > > Could someone take a look at this patch please ? This is for addressing > > this ticket https://trac.ffmpeg.org/ticket/7297 > > If this patch fixes ticket 7297 then the commit message should say so > > the option also may need documentation unless its private (which it is not > currently) its listed in -h full > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > "You are 36 times more likely to die in a bathtub than at the hands of a > terrorist. Also, you are 2.5 times more likely to become a president and > 2 times more likely to become an astronaut, than to die in a terrorist > attack." -- Thoughty2 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".