Hi guys, First attempt at $subj. Feng works, but Darwin still hangs. First three are related to http, the last adds actual tunnelling support to rtsp. Josh
On 06/03/2010 12:59 AM, Josh Allmann wrote: [rtsp-http] Hi, here some comments, I used your git to checkout* your branch, then I used git format-patch to have a better look at your changes, here my comments. Added URL_DELAYOPEN flag to internal api 6322eb07: +#define URL_DELAYOPEN 4 /* follows URL_* scheme defined in avio.h */ I'm not sure outside programs have no use for it. +++ b/libavformat/http.h @@ -0,0 +1,33 @@ +/* + * HTTP definitions + * Copyright (c) 2002 Fabrice Bellard Doesn't look right. Added in capability to write custom HTTP headers. ea2d753ba: + unsigned char headers[BUFFER_SIZE]; +void ff_http_set_headers(URLContext *h, const char *headers) +{ + HTTPContext *s = h->priv_data; + snprintf(s->headers, sizeof(s->headers), "%s", headers); +} I'm not completely happy with this, probably a way to set/get them might be more nice, but that could be done on a later time. Added support for disabling chunked transfer encoding. 1ee35e509: Seems ok Added in RTSP tunneling over HTTP. 2f6e33908: + if (!av_base64_encode(base64buf, sizeof(base64buf), buf, strlen(buf))) { + av_log(s, AV_LOG_ERROR, "Unable to base64 encode RTSP.\n"); + return; + } currently this function assumes that it cannot fail, probably we should propagate errors. +static inline void close_cxn(RTSPState *rt) It's a nit but I dislike cxn The rest looks good. lu * If you have already your own git tree you can do the following: # set the remote to just track the rtsp-tunneling branch git remote add -t rtsp-tunneling j0sh git://github.com/j0sh/ffmpeg-soc.git # fetch it git fetch j0sh #checkout git checkout -b rtsp-tunneling j0sh/rtsp-tunneling now you can use git pull to update it. -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On 3 June 2010 07:34, Luca Barbato <lu_zero@gentoo.org> wrote:
On 06/03/2010 12:59 AM, Josh Allmann wrote: [rtsp-http]
Hi, here some comments, I used your git to checkout* your branch, then I used git format-patch to have a better look at your changes, here my comments.
Added URL_DELAYOPEN flag to internal api 6322eb07:
+#define URL_DELAYOPEN 4 /* follows URL_* scheme defined in avio.h */
I'm not sure outside programs have no use for it.
Removed per Ronald's suggestion.
+++ b/libavformat/http.h @@ -0,0 +1,33 @@ +/* + * HTTP definitions + * Copyright (c) 2002 Fabrice Bellard
Doesn't look right.
Fixed (although I don't really feel like I deserve copyright to the whole file)
Added in capability to write custom HTTP headers. ea2d753ba:
+ unsigned char headers[BUFFER_SIZE];
+void ff_http_set_headers(URLContext *h, const char *headers) +{ + HTTPContext *s = h->priv_data; + snprintf(s->headers, sizeof(s->headers), "%s", headers); +}
I'm not completely happy with this, probably a way to set/get them might be more nice, but that could be done on a later time.
Nobody is happy about this one but it's the quickest solution for now.
Added support for disabling chunked transfer encoding. 1ee35e509:
Seems ok
Added in RTSP tunneling over HTTP. 2f6e33908:
+ if (!av_base64_encode(base64buf, sizeof(base64buf), buf, strlen(buf))) { + av_log(s, AV_LOG_ERROR, "Unable to base64 encode RTSP.\n"); + return; + }
currently this function assumes that it cannot fail, probably we should propagate errors.
Done, added a fifth patch for this. I only check and propagate errors within ff_rtsp_send_cmd* functions, but I don't do anything about them when called from other ff_rtsp* functions.
+static inline void close_cxn(RTSPState *rt)
It's a nit but I dislike cxn
The rest looks good.
lu
* If you have already your own git tree you can do the following: # set the remote to just track the rtsp-tunneling branch git remote add -t rtsp-tunneling j0sh git://github.com/j0sh/ffmpeg-soc.git
# fetch it git fetch j0sh
#checkout git checkout -b rtsp-tunneling j0sh/rtsp-tunneling
now you can use git pull to update it.
git rocks.
On Fri, 4 Jun 2010, Josh Allmann wrote:
On 3 June 2010 07:34, Luca Barbato <lu_zero@gentoo.org> wrote:
+ if (!av_base64_encode(base64buf, sizeof(base64buf), buf, strlen(buf))) { + av_log(s, AV_LOG_ERROR, "Unable to base64 encode RTSP.\n"); + return; + }
currently this function assumes that it cannot fail, probably we should propagate errors.
Done, added a fifth patch for this. I only check and propagate errors within ff_rtsp_send_cmd* functions, but I don't do anything about them when called from other ff_rtsp* functions.
Propagating errors generally is a very good idea. For this particular case, the only thing that could fail with base64 is that the output buffer isn't large enough, and we already make sure it's large enough. But I'm ok with it either way. // Martin
On 4 June 2010 02:39, Martin Storsjö <martin@martin.st> wrote:
On Fri, 4 Jun 2010, Josh Allmann wrote:
On 3 June 2010 07:34, Luca Barbato <lu_zero@gentoo.org> wrote:
+ if (!av_base64_encode(base64buf, sizeof(base64buf), buf, strlen(buf))) { + av_log(s, AV_LOG_ERROR, "Unable to base64 encode RTSP.\n"); + return; + }
currently this function assumes that it cannot fail, probably we should propagate errors.
Done, added a fifth patch for this. I only check and propagate errors within ff_rtsp_send_cmd* functions, but I don't do anything about them when called from other ff_rtsp* functions.
Propagating errors generally is a very good idea. For this particular case, the only thing that could fail with base64 is that the output buffer isn't large enough, and we already make sure it's large enough. But I'm ok with it either way.
Fixed that, and other minor errors (s/handler/handle, spelling fixes in the proper commit, etc). Soc repo has been updated as well. I am starting another thread that should address the issue of the subsequent HTTP writes not updating the header. Josh
On 5 June 2010 01:28, Josh Allmann <joshua.allmann@gmail.com> wrote:
On 4 June 2010 02:39, Martin Storsjö <martin@martin.st> wrote:
On Fri, 4 Jun 2010, Josh Allmann wrote:
I am starting another thread that should address the issue of the subsequent HTTP writes not updating the header.
Until the other thread on the dictionary gets resolved, this patch should make everything dandy. The only thing that's missing now is the ability to extract the x-server-ip from the response... but other than that, everything is good to go.
Josh
Hi, On Sat, Jun 5, 2010 at 4:28 AM, Josh Allmann <joshua.allmann@gmail.com> wrote:
Fixed that, and other minor errors (s/handler/handle, spelling fixes in the proper commit, etc). Soc repo has been updated as well. [..] + if (!s->init) { + int ret = http_open_cnx(h); + if (ret != 0) { + av_free (s);
The av_free(s) should be removed in read/write, otherwise subsequent calls to read/write (or close) will/might crash.
+/** + * Sets custom HTTP headers. Note this overwrites all default options. + * + * The following headers will always be set, no matter what: + * -HTTP verb, path, HTTP version + * -User-Agent + * -Transfer-Encoding (if applicable) + * - Authorization (if applicable) + * + * The following headers are set by default, and will be overwritten if + * custom headers are set. Be sure to re-specify them if needed. + * -Accept + * -Range + * -Host + * -Connection + * + * @param h URL context for this HTTP connection + * @param is_chunked 0 to disable chunking, nonzero otherwise. + */ +void ff_http_set_headers(URLContext *h, const char *headers);
You shouldn't omit them if missing from the "custom" header. The expected behaviour if custom header is "Accept: bla\r\n" is that the default Host is kept (same for the others). Also, please document that the caller is required to maintain trailing "\r\n" in the custom header. Assert this in the implementation. Also document that "\0" resets the custom header.
@@ -391,7 +398,7 @@ static int http_read(URLContext *h, uint8_t *buf, int size) /* used only when posting data */ static int http_write(URLContext *h, const uint8_t *buf, int size) { - char temp[11]; /* 32-bit hex + CRLF + nul */ + char temp[11] = ""; /* 32-bit hex + CRLF + nul */ int ret; char crlf[] = "\r\n"; HTTPContext *s = h->priv_data; @@ -413,11 +420,12 @@ static int http_write(URLContext *h, const uint8_t *buf, int size) * signal EOF */ if (size > 0) { /* upload data using chunked encoding */ + if(s->is_chunked) snprintf(temp, sizeof(temp), "%x\r\n", size);
if ((ret = url_write(s->hd, temp, strlen(temp))) < 0 ||
is_chunked && (ret = url_write... More generally, feel free to unroll this into: if (is_chunked){ snprintf(); url_write(); } url_write(); if (is_chunked) url_write(); How does the caller or callee define the length of the data if it's not chunked? We need some defined way of getting that, otherwise this will break depending on the receiving server implementation. #4 is fine, if Luca is OK (or Martin), please apply.
+ /** RTSP transport mode, such as plain or tunneled. */ + int mode;
enum RTSPMode mode. RTSPMode is also a little generic, but I don't really have suggestions for a better name. Luca? RTSPProtocolConnectionMode? I think #6 is OK, again Luca/Martin for second review and then it can be applied. Ronald
On Sat, 5 Jun 2010, Ronald S. Bultje wrote:
How does the caller or callee define the length of the data if it's not chunked? We need some defined way of getting that, otherwise this will break depending on the receiving server implementation.
If you don't do chunked, you either have to signal it in the Content-Length header of the POST, or simply cut the connection when you're done. So for normal use of the http protocol in write mode, only chunked mode, or no Content-Length header at all would make sense. For the RTSP HTTP tunneling, the spec suggests a Content-Length with a large value, for compatibility with more proxies, though...
#4 is fine, if Luca is OK (or Martin), please apply.
Yes, this looks fine to me, too. Will apply in a bit if Luca doesn't have any objections.
+ /** RTSP transport mode, such as plain or tunneled. */ + int mode;
enum RTSPMode mode. RTSPMode is also a little generic, but I don't really have suggestions for a better name. Luca? RTSPProtocolConnectionMode?
RTSPSessionMode, RTSPChannelMode, RTSPControlChannelMode are other suggestions, but I'm not really sure which one I like the best.
I think #6 is OK, again Luca/Martin for second review and then it can be applied.
This one looks fine to me, too. // Martin
On 06/05/2010 09:15 PM, Martin Storsjö wrote:
Yes, this looks fine to me, too. Will apply in a bit if Luca doesn't have any objections.
Go ahead =) lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On Sat, 5 Jun 2010, Luca Barbato wrote:
On 06/05/2010 09:15 PM, Martin Storsjö wrote:
Yes, this looks fine to me, too. Will apply in a bit if Luca doesn't have any objections.
Go ahead =)
Applied #4 and #6. Good work Josh with having the first few patches applied already. :-) // Martin
On 06/05/2010 09:15 PM, Martin Storsjö wrote:
RTSPSessionMode, RTSPChannelMode, RTSPControlChannelMode are other suggestions, but I'm not really sure which one I like the best.
RTSPTransport
This one looks fine to me, too.
ok for me as well. -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
Hi, On Sat, Jun 5, 2010 at 3:26 PM, Luca Barbato <lu_zero@gentoo.org> wrote:
On 06/05/2010 09:15 PM, Martin Storsjö wrote:
RTSPSessionMode, RTSPChannelMode, RTSPControlChannelMode are other suggestions, but I'm not really sure which one I like the best.
RTSPTransport
It's not a transport, RDT/RTP is a transport, no? This is control-connection-transport. :-). Let's call it RTSPControlTransport? Ronald
On Sat, 5 Jun 2010, Ronald S. Bultje wrote:
On Sat, Jun 5, 2010 at 3:26 PM, Luca Barbato <lu_zero@gentoo.org> wrote:
On 06/05/2010 09:15 PM, Martin Storsjö wrote:
RTSPSessionMode, RTSPChannelMode, RTSPControlChannelMode are other suggestions, but I'm not really sure which one I like the best.
RTSPTransport
It's not a transport, RDT/RTP is a transport, no? This is control-connection-transport. :-). Let's call it RTSPControlTransport?
Ok with me. Since there are many things that can be transported in many different ways, being (overly) clear is only a good thing. :-) // Martin
On 06/05/2010 09:32 PM, Martin Storsjö wrote:
different ways, being (overly) clear is only a good thing. :-) Amen =)
lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On 5 June 2010 12:01, Ronald S. Bultje <rsbultje@gmail.com> wrote:
Hi,
On Sat, Jun 5, 2010 at 4:28 AM, Josh Allmann <joshua.allmann@gmail.com> wrote:
Fixed that, and other minor errors (s/handler/handle, spelling fixes in the proper commit, etc). Soc repo has been updated as well. [..] + if (!s->init) { + int ret = http_open_cnx(h); + if (ret != 0) { + av_free (s);
The av_free(s) should be removed in read/write, otherwise subsequent calls to read/write (or close) will/might crash.
Fixed.
+/** + * Sets custom HTTP headers. Note this overwrites all default options. + * + * The following headers will always be set, no matter what: + * -HTTP verb, path, HTTP version + * -User-Agent + * -Transfer-Encoding (if applicable) + * - Authorization (if applicable) + * + * The following headers are set by default, and will be overwritten if + * custom headers are set. Be sure to re-specify them if needed. + * -Accept + * -Range + * -Host + * -Connection + * + * @param h URL context for this HTTP connection + * @param is_chunked 0 to disable chunking, nonzero otherwise. + */ +void ff_http_set_headers(URLContext *h, const char *headers);
You shouldn't omit them if missing from the "custom" header. The expected behaviour if custom header is "Accept: bla\r\n" is that the default Host is kept (same for the others). Also, please document that the caller is required to maintain trailing "\r\n" in the custom header. Assert this in the implementation. Also document that "\0" resets the custom header.
Fixed on all counts. I was wrong earlier; adding in the default headers don't break stuff, so we don't need to omit anything.
@@ -391,7 +398,7 @@ static int http_read(URLContext *h, uint8_t *buf, int size) /* used only when posting data */ static int http_write(URLContext *h, const uint8_t *buf, int size) { - char temp[11]; /* 32-bit hex + CRLF + nul */ + char temp[11] = ""; /* 32-bit hex + CRLF + nul */ int ret; char crlf[] = "\r\n"; HTTPContext *s = h->priv_data; @@ -413,11 +420,12 @@ static int http_write(URLContext *h, const uint8_t *buf, int size) * signal EOF */ if (size > 0) { /* upload data using chunked encoding */ + if(s->is_chunked) snprintf(temp, sizeof(temp), "%x\r\n", size);
if ((ret = url_write(s->hd, temp, strlen(temp))) < 0 ||
is_chunked && (ret = url_write... More generally, feel free to unroll this into: if (is_chunked){ snprintf(); url_write(); } url_write(); if (is_chunked) url_write();
Fixed. The code is not as tight now, but a little less gnarly.
How does the caller or callee define the length of the data if it's not chunked? We need some defined way of getting that, otherwise this will break depending on the receiving server implementation.
#4 is fine, if Luca is OK (or Martin), please apply.
+ /** RTSP transport mode, such as plain or tunneled. */ + int mode;
enum RTSPMode mode. RTSPMode is also a little generic, but I don't really have suggestions for a better name. Luca? RTSPProtocolConnectionMode?
Changed to RTSPControlTransport
I think #6 is OK, again Luca/Martin for second review and then it can be applied.
Ronald _______________________________________________ FFmpeg-soc mailing list FFmpeg-soc@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
Sent the wrong patches, use these instead. Also, my github rtsp-tunneling branch has been updated. On 5 June 2010 14:59, Josh Allmann <joshua.allmann@gmail.com> wrote:
On 5 June 2010 12:01, Ronald S. Bultje <rsbultje@gmail.com> wrote:
Hi,
On Sat, Jun 5, 2010 at 4:28 AM, Josh Allmann <joshua.allmann@gmail.com> wrote:
Fixed that, and other minor errors (s/handler/handle, spelling fixes in the proper commit, etc). Soc repo has been updated as well. [..] + if (!s->init) { + int ret = http_open_cnx(h); + if (ret != 0) { + av_free (s);
The av_free(s) should be removed in read/write, otherwise subsequent calls to read/write (or close) will/might crash.
Fixed.
+/** + * Sets custom HTTP headers. Note this overwrites all default options. + * + * The following headers will always be set, no matter what: + * -HTTP verb, path, HTTP version + * -User-Agent + * -Transfer-Encoding (if applicable) + * - Authorization (if applicable) + * + * The following headers are set by default, and will be overwritten if + * custom headers are set. Be sure to re-specify them if needed. + * -Accept + * -Range + * -Host + * -Connection + * + * @param h URL context for this HTTP connection + * @param is_chunked 0 to disable chunking, nonzero otherwise. + */ +void ff_http_set_headers(URLContext *h, const char *headers);
You shouldn't omit them if missing from the "custom" header. The expected behaviour if custom header is "Accept: bla\r\n" is that the default Host is kept (same for the others). Also, please document that the caller is required to maintain trailing "\r\n" in the custom header. Assert this in the implementation. Also document that "\0" resets the custom header.
Fixed on all counts. I was wrong earlier; adding in the default headers don't break stuff, so we don't need to omit anything.
@@ -391,7 +398,7 @@ static int http_read(URLContext *h, uint8_t *buf, int size) /* used only when posting data */ static int http_write(URLContext *h, const uint8_t *buf, int size) { - char temp[11]; /* 32-bit hex + CRLF + nul */ + char temp[11] = ""; /* 32-bit hex + CRLF + nul */ int ret; char crlf[] = "\r\n"; HTTPContext *s = h->priv_data; @@ -413,11 +420,12 @@ static int http_write(URLContext *h, const uint8_t *buf, int size) * signal EOF */ if (size > 0) { /* upload data using chunked encoding */ + if(s->is_chunked) snprintf(temp, sizeof(temp), "%x\r\n", size);
if ((ret = url_write(s->hd, temp, strlen(temp))) < 0 ||
is_chunked && (ret = url_write... More generally, feel free to unroll this into: if (is_chunked){ snprintf(); url_write(); } url_write(); if (is_chunked) url_write();
Fixed. The code is not as tight now, but a little less gnarly.
How does the caller or callee define the length of the data if it's not chunked? We need some defined way of getting that, otherwise this will break depending on the receiving server implementation.
#4 is fine, if Luca is OK (or Martin), please apply.
+ /** RTSP transport mode, such as plain or tunneled. */ + int mode;
enum RTSPMode mode. RTSPMode is also a little generic, but I don't really have suggestions for a better name. Luca? RTSPProtocolConnectionMode?
Changed to RTSPControlTransport
I think #6 is OK, again Luca/Martin for second review and then it can be applied.
Ronald _______________________________________________ FFmpeg-soc mailing list FFmpeg-soc@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
Hi, On Sat, Jun 5, 2010 at 6:05 PM, Josh Allmann <joshua.allmann@gmail.com> wrote:
+ HTTPContext *s = h->priv_data; + int len = strlen(headers); + + if (len && strcmp("\r\n", headers+len-2)) + av_log(NULL, AV_LOG_ERROR, "No trailing CRLF found in HTTP header.\n"); + + snprintf(s->headers, sizeof(s->headers), "%s", headers);
av_strlcpy().
+ /* set default headers if needed */ + if(!strstr(s->headers, "\r\nUser-Agent:")) + len += snprintf(headers+len, sizeof(headers)-len, "User-Agent: %s\r\n", LIBAVFORMAT_IDENT); + if(!strstr(s->headers, "\r\nAccept:"))
if (strncmp(.., "Accept:", 7) && !strstr(.., "\r\nAccept:")) .. Same for the others. Otherwise you'll miss the first header. A macro or utility function for this would of course be OK: #define has_header(str, hdr, size) \ (strncmp(str, hdr, size) && !strstr(.., "\r\n" hdr)) Something like that should compile.
+ /* now add in custom headers */ + snprintf(headers+len, sizeof(headers)-len, "%s", s->headers);
Maybe use av_strlcat(). Also the "%s" is again not needed, and this might need an if (s->headers[0]). #3 is OK. You have two #5 patches, which one is right? Ronald
On 6 June 2010 13:44, Ronald S. Bultje <rsbultje@gmail.com> wrote:
Hi,
On Sat, Jun 5, 2010 at 6:05 PM, Josh Allmann <joshua.allmann@gmail.com> wrote:
+ HTTPContext *s = h->priv_data; + int len = strlen(headers); + + if (len && strcmp("\r\n", headers+len-2)) + av_log(NULL, AV_LOG_ERROR, "No trailing CRLF found in HTTP header.\n"); + + snprintf(s->headers, sizeof(s->headers), "%s", headers);
av_strlcpy().
Fixed. I also changed all of my other calls to use av_str* equivalents.
+ /* set default headers if needed */ + if(!strstr(s->headers, "\r\nUser-Agent:")) + len += snprintf(headers+len, sizeof(headers)-len, "User-Agent: %s\r\n", LIBAVFORMAT_IDENT); + if(!strstr(s->headers, "\r\nAccept:"))
if (strncmp(.., "Accept:", 7) && !strstr(.., "\r\nAccept:")) ..
Same for the others. Otherwise you'll miss the first header. A macro or utility function for this would of course be OK:
#define has_header(str, hdr, size) \ (strncmp(str, hdr, size) && !strstr(.., "\r\n" hdr))
Something like that should compile.
Good catch. Used av_stristart(...) || av_stristr(...) in an inlined function.
+ /* now add in custom headers */ + snprintf(headers+len, sizeof(headers)-len, "%s", s->headers);
Maybe use av_strlcat(). Also the "%s" is again not needed, and this might need an if (s->headers[0]).
Changed to av_strcpy since we know where we're starting from (avoids a call to strlen). Copying a null string should yield a null string without any problems.
#3 is OK.
Do you think a more generic ff_http_ignore_header function would be better at avoiding API sprawl? We can special-case "Transfer-Encoding" in there and set the flag then.
You have two #5 patches, which one is right?
Whoops, I attached #5 instead of #1.
Ronald _______________________________________________ FFmpeg-soc mailing list FFmpeg-soc@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
Hi, #1: On Sun, Jun 6, 2010 at 10:10 PM, Josh Allmann <joshua.allmann@gmail.com> wrote:
+ if (ret != 0) { + return ret; + }
Superfluous {}.
+ /* A size of zero can be used to force + * initializaton of the connection. */ + if (!size) + return 0;
Eh... I'm not sure I like that. #2:
+static inline int has_header(const char *str, const char *header) +{ + return av_stristart(str, header+2, &str) || av_stristr(str, header); +}
av_stristart(a, b, NULL) should work also. Why header+2 (I know it's because of \r\n, but please document that because we'll forget...)? #5:
av_strlcat(buf, str, sizeof(buf)); av_free(str); } + if (send_content_length > 0 && send_content) av_strlcatf(buf, sizeof(buf), "Content-Length: %d\r\n", send_content_length);
Cosmetic. The rest looks nice, I hope we can apply in the next round. Ronald
On 7 June 2010 08:16, Ronald S. Bultje <rsbultje@gmail.com> wrote:
Hi,
#1:
On Sun, Jun 6, 2010 at 10:10 PM, Josh Allmann <joshua.allmann@gmail.com> wrote:
+ if (ret != 0) { + return ret; + }
Superfluous {}.
Removed.
+ /* A size of zero can be used to force + * initializaton of the connection. */ + if (!size) + return 0;
Eh... I'm not sure I like that.
Left as per IRC discussion.
#2:
+static inline int has_header(const char *str, const char *header) +{ + return av_stristart(str, header+2, &str) || av_stristr(str, header); +}
av_stristart(a, b, NULL) should work also. Why header+2 (I know it's because of \r\n, but please document that because we'll forget...)?
#5:
av_strlcat(buf, str, sizeof(buf)); av_free(str); } + if (send_content_length > 0 && send_content) av_strlcatf(buf, sizeof(buf), "Content-Length: %d\r\n", send_content_length);
Cosmetic.
Removed.
The rest looks nice, I hope we can apply in the next round.
*dances* Luca, Martin, any more comments? Josh
Hi, On Mon, Jun 7, 2010 at 12:37 PM, Josh Allmann <joshua.allmann@gmail.com> wrote:
On 7 June 2010 08:16, Ronald S. Bultje <rsbultje@gmail.com> wrote:
The rest looks nice, I hope we can apply in the next round.
*dances* Luca, Martin, any more comments?
Current patches fine with me. Ronald
On 06/07/2010 06:37 PM, Josh Allmann wrote:
*dances* Luca, Martin, any more comments?
Looks ok, maybe if we'll implement another tunneled mode (e.g. rtsp over tls and srtp) the enum might change its name, but is something we'll consider later/post-soc. lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
Hi Josh, On Jun 7, 2010, at 18:37, Josh Allmann wrote:
The rest looks nice, I hope we can apply in the next round.
*dances* Luca, Martin, any more comments?
Great that both Ronald and Luca like it! I'm a bit unavailable at the moment, I'm on the move with very limited net connectivity. I'll read it through tomorrow and apply if there's nothing I object to :-) // Martin
On Mon, 7 Jun 2010, Josh Allmann wrote:
Luca, Martin, any more comments?
#1 looked ok, applied. Overpedantic nit: The norm for commit messages seems to be imperative form, e.g. "Rewrite", "Fix", "Change", instead of describing the commit in retrospect saying what you did. #2, some minor nits:
@@ -46,12 +47,23 @@ typedef struct { char location[URL_SIZE]; HTTPAuthState auth_state; int init; + unsigned char headers[BUFFER_SIZE]; } HTTPContext;
The size of this field isn't related at all to the buffer size, so reusing BUFFER_SIZE feels wrong IMO.
+void ff_http_set_headers(URLContext *h, const char *headers) +{ + HTTPContext *s = h->priv_data; + int len = strlen(headers); + + if (len && strcmp("\r\n", headers+len-2))
Some space around the operators would be nice.
+static inline int has_header(const char *str, const char *header) +{ + /* header+2 to skip over CRLF prefix. (make sure you have one!) */ + return av_stristart(str, header+2, NULL) || av_stristr(str, header); +}
This could use spaces around the +, too.
+ + /* set default headers if needed */ + if(!has_header(s->headers, "\r\nUser-Agent: ")) + len += av_strlcatf(headers+len, sizeof(headers)-len, "User-Agent: %s\r\n", LIBAVFORMAT_IDENT); + if(!has_header(s->headers, "\r\nAccept: ")) + len += av_strlcpy(headers+len, "Accept: */*\r\n", sizeof(headers)-len); + if(!has_header(s->headers, "\r\nRange: ")) + len += av_strlcatf(headers+len, sizeof(headers)-len, "Range: bytes=%"PRId64"\r\n", s->off); + if(!has_header(s->headers, "\r\nConnection: ")) + len += av_strlcpy(headers+len, "Connection: close\r\n", sizeof(headers)-len); + if(!has_header(s->headers, "\r\nHost: ")) + len += av_strlcatf(headers+len, sizeof(headers)-len, "Host: %s\r\n", hoststr);
Space between if and the parenthesis, the first if and the comment is off. Also, if possible, try to keep lines below 80 characters in length. Also, mixing av_strlcpy and av_strlcatf makes it hard to read even if it's slightly more efficient this way.
+/** + * Sets custom HTTP headers. Note this overwrites all default options.
Not true any more?
+ * A trailing CRLF ("\r\n") is required for custom headers. + * Passing in an empty header string ("\0") will reset to defaults. + * + * The following headers can be overriden by custom values, + * otherwise they will be set to their defaults. + * -User-Agent + * -Accept + * -Range + * -Host + * -Connection + * + * @param h URL context for this HTTP connection + * @param is_chunked 0 to disable chunking, nonzero otherwise.
Uhm, copypaste error? :-) Applied with most of the nits adjusted. (later, after writing the above) And applied bugfixes after noticing them after applying the patches. Please test error handling in code you modify too, not just the normal cases. Preferrably, test it under valgrind too, to catch the use of uninitialized buffers. You want to catch those bugs yourself, so I don't have to. :-) Ronald, Josh: What about adding if (!s->hd) return AVERROR(EIO); after the if (!s->init) check? Currently, if the delayed open fails, you'd get a crash on the second url_read(), if the calling code doesn't check the return value from every single call and avoid touching it again after an error. Earlier, after a successful url_open(), there was always a working s->hd initialized. Also, Josh, in your patch, you said that the connection is initialized on http_read, *write and *seek, but I'm not sure if http_seek handles being called before the first url_read/url_write. As the very minimum, it requires an if (old_hd) before the url_close at the end. Now on to the rest of the patches... // Martin
On Tue, 8 Jun 2010, Martin Storsjö wrote:
Ronald, Josh: What about adding if (!s->hd) return AVERROR(EIO); after the if (!s->init) check? Currently, if the delayed open fails, you'd get a crash on the second url_read(), if the calling code doesn't check the
Actually it didn't crash (immediately), but still malfunctioned, so I added such checks.
return value from every single call and avoid touching it again after an error. Earlier, after a successful url_open(), there was always a working s->hd initialized.
Also, Josh, in your patch, you said that the connection is initialized on http_read, *write and *seek, but I'm not sure if http_seek handles being called before the first url_read/url_write. As the very minimum, it requires an if (old_hd) before the url_close at the end.
Added similar checks for http_seek too, fixing a regression with e.g. the image2 demuxer that does an url_fsize() before any read had been done. Ronald: There's still an arcitectural regression introduced by the http delay open, that I'm a bit unsure about how to fix. url_fdopen (which is invoked directly by url_fopen) copies the value of is_streamed to the ByteIOContext directly after opening. In the http protocol, is_streamed is 1 initially to indicate that seeking isn't supported, but is set to 0 if a proper reply header is found. This used to be done already within url_open(), but isn't anymore since the connection hasn't been opened yet. So this effectively removes support for seeking in http sources for now. How do you suggest we fix this? I'm not sure if this is the last issue - the http delay open issue feels like a can of worms/regressions at the moment. One option would be to disable delay opening by default and only enable it in the RTSP tunneling case by a private flag or something similar. // Martin
On 6/9/10 11:28 AM, Martin Storsjö wrote:
How do you suggest we fix this?
I'm not sure if this is the last issue - the http delay open issue feels like a can of worms/regressions at the moment. One option would be to disable delay opening by default and only enable it in the RTSP tunneling case by a private flag or something similar.
the simplest way seems that one, otherwise we could have the protocol actually split open and connect at interface level... lu
Hi, On Wed, Jun 9, 2010 at 5:28 AM, Martin Storsjö <martin@martin.st> wrote:
Ronald: There's still an arcitectural regression introduced by the http delay open, that I'm a bit unsure about how to fix. url_fdopen (which is invoked directly by url_fopen) copies the value of is_streamed to the ByteIOContext directly after opening. In the http protocol, is_streamed is 1 initially to indicate that seeking isn't supported, but is set to 0 if a proper reply header is found. This used to be done already within url_open(), but isn't anymore since the connection hasn't been opened yet. So this effectively removes support for seeking in http sources for now.
How do you suggest we fix this?
This, and many other regressions, are all caused because URL* combines allocation with opening, something that AVFormatContext/AVCodecContext separate. The proper thing to do is to separate it also. That breaks or adds API, which is bad, but maybe we just have to byte the bullet. It also means we have to separate close and free. Michael, that OK with you, if it doesn't break existing API/apps?
I'm not sure if this is the last issue - the http delay open issue feels like a can of worms/regressions at the moment. One option would be to disable delay opening by default and only enable it in the RTSP tunneling case by a private flag or something similar.
No, no private flags will be introduced to maintain this. We said that this is a "quick and little bit dirty" way to get Josh's stuff moving, and that's what it is. Once we have a sane solution, we'll remove this (ff_http_*()) again anyway, e.g. in favour of AVOption or whatever. Ronald
On Wed, 9 Jun 2010, Ronald S. Bultje wrote:
On Wed, Jun 9, 2010 at 5:28 AM, Martin Storsjö <martin@martin.st> wrote:
Ronald: There's still an arcitectural regression introduced by the http delay open, that I'm a bit unsure about how to fix. url_fdopen (which is invoked directly by url_fopen) copies the value of is_streamed to the ByteIOContext directly after opening. In the http protocol, is_streamed is 1 initially to indicate that seeking isn't supported, but is set to 0 if a proper reply header is found. This used to be done already within url_open(), but isn't anymore since the connection hasn't been opened yet. So this effectively removes support for seeking in http sources for now.
How do you suggest we fix this?
This, and many other regressions, are all caused because URL* combines allocation with opening, something that AVFormatContext/AVCodecContext separate. The proper thing to do is to separate it also. That breaks or adds API, which is bad, but maybe we just have to byte the bullet. It also means we have to separate close and free.
Michael, that OK with you, if it doesn't break existing API/apps?
Does adding things to URLContext break the ABI? The external API could be kept unmodified here - as Ronald said on IRC, we could add individual functions, like this: url_open = url_alloc + url_connect url_close = url_disconnect + url_free // Martin
Hi, On Wed, Jun 9, 2010 at 2:21 PM, Martin Storsjö <martin@martin.st> wrote:
Does adding things to URLContext break the ABI?
No, see the doxy: " * New fields can be added to the end with minor version bumps." Ronald
On 6/9/10 8:24 PM, Ronald S. Bultje wrote:
Hi,
On Wed, Jun 9, 2010 at 2:21 PM, Martin Storsjö<martin@martin.st> wrote:
Does adding things to URLContext break the ABI?
No, see the doxy: " * New fields can be added to the end with minor version bumps."
then let's split that, I'll have a look tomorrow morning probably, BTW we might look if some network code could be factorized somehow. lu
On Wed, Jun 09, 2010 at 01:53:51PM -0400, Ronald S. Bultje wrote:
Hi,
On Wed, Jun 9, 2010 at 5:28 AM, Martin Storsjö <martin@martin.st> wrote:
Ronald: There's still an arcitectural regression introduced by the http delay open, that I'm a bit unsure about how to fix. url_fdopen (which is invoked directly by url_fopen) copies the value of is_streamed to the ByteIOContext directly after opening. In the http protocol, is_streamed is 1 initially to indicate that seeking isn't supported, but is set to 0 if a proper reply header is found. This used to be done already within url_open(), but isn't anymore since the connection hasn't been opened yet. So this effectively removes support for seeking in http sources for now.
How do you suggest we fix this?
This, and many other regressions, are all caused because URL* combines allocation with opening, something that AVFormatContext/AVCodecContext separate. The proper thing to do is to separate it also. That breaks or adds API, which is bad, but maybe we just have to byte the bullet. It also means we have to separate close and free.
Michael, that OK with you, if it doesn't break existing API/apps?
the only thing needed for url_open() / url_open_protocol() is to pass a allocated context and add a flag that indicates that the context has already been allocated then we need a function like avformat_alloc_context() for allocation spliting free/close does not seem required this leaves url_fopen() which iam not sure how to handle best [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator
On Fri, 11 Jun 2010, Michael Niedermayer wrote:
On Wed, Jun 09, 2010 at 01:53:51PM -0400, Ronald S. Bultje wrote:
This, and many other regressions, are all caused because URL* combines allocation with opening, something that AVFormatContext/AVCodecContext separate. The proper thing to do is to separate it also. That breaks or adds API, which is bad, but maybe we just have to byte the bullet. It also means we have to separate close and free.
Michael, that OK with you, if it doesn't break existing API/apps?
the only thing needed for url_open() / url_open_protocol() is to pass a allocated context and add a flag that indicates that the context has already been allocated
then we need a function like avformat_alloc_context() for allocation
In general, that sounds like a good solution. For this particular case, the options that we would like to set are http specific, and (with the current design) are to be set within HTTPContext, that is allocated and set to URLContext->priv_data only in http_open (and its size isn't even known outside of http.c. So that would require some restructuring - probably on the lines of setting format-specific options on (de)muxers, that we discussed a few months ago.
this leaves url_fopen() which iam not sure how to handle best
For this case, I don't think it requires any special changes. In those cases you probably don't want to set any specific details on the underlying URLContext. If you do want that, you can set up the URLContext yourself and do url_fdopen. // Martin
On Mon, Jun 14, 2010 at 11:36:07AM +0300, Martin Storsjö wrote:
On Fri, 11 Jun 2010, Michael Niedermayer wrote:
On Wed, Jun 09, 2010 at 01:53:51PM -0400, Ronald S. Bultje wrote:
This, and many other regressions, are all caused because URL* combines allocation with opening, something that AVFormatContext/AVCodecContext separate. The proper thing to do is to separate it also. That breaks or adds API, which is bad, but maybe we just have to byte the bullet. It also means we have to separate close and free.
Michael, that OK with you, if it doesn't break existing API/apps?
the only thing needed for url_open() / url_open_protocol() is to pass a allocated context and add a flag that indicates that the context has already been allocated
then we need a function like avformat_alloc_context() for allocation
In general, that sounds like a good solution.
For this particular case, the options that we would like to set are http specific, and (with the current design) are to be set within HTTPContext, that is allocated and set to URLContext->priv_data only in http_open (and its size isn't even known outside of http.c. So that would require some
add a URLProtocol.priv_data_size this also might simplify existing code by moving allocation of priv_data to a common place [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
Hi, On Mon, Jun 14, 2010 at 12:47 PM, Michael Niedermayer <michaelni@gmx.at> wrote:
add a URLProtocol.priv_data_size this also might simplify existing code by moving allocation of priv_data to a common place
This doesn't solve the problem of fields that should be initialized to a non-zero state in protocol_open(). We really need an API where we have: 1) a function that allocates both URLContext as well as priv_data context (so the type of protocol, e.g. HTTP, is known at this point) 2) a function that initializes these values to a default state; it makes sense that this function is called at the same time as 1. It also might make sense that the URI is known at this point 3) a window here where we can make changes to this context by calling functions or eventually by introducing AVOptions 4) the moment where we actually open the connection, which would be url_open(). Ronald
On Mon, 14 Jun 2010, Ronald S. Bultje wrote:
On Mon, Jun 14, 2010 at 12:47 PM, Michael Niedermayer <michaelni@gmx.at> wrote:
add a URLProtocol.priv_data_size this also might simplify existing code by moving allocation of priv_data to a common place
This doesn't solve the problem of fields that should be initialized to a non-zero state in protocol_open(). We really need an API where we have: 1) a function that allocates both URLContext as well as priv_data context (so the type of protocol, e.g. HTTP, is known at this point) 2) a function that initializes these values to a default state; it makes sense that this function is called at the same time as 1. It also might make sense that the URI is known at this point 3) a window here where we can make changes to this context by calling functions or eventually by introducing AVOptions
Additionally, as far as I've understood AVOptions, they can only set something within one struct, since it simply uses offsets. For this case, we'd need AVOptions that would only apply if the URLContext happens to be a HTTP context, and then set the options within the priv_data, which can't be done as a simple offset (unless the priv_data is tacked on at the end of the context...) // Martin
On Mon, Jun 14, 2010 at 08:12:30PM +0300, Martin Storsjö wrote:
On Mon, 14 Jun 2010, Ronald S. Bultje wrote:
On Mon, Jun 14, 2010 at 12:47 PM, Michael Niedermayer <michaelni@gmx.at> wrote:
add a URLProtocol.priv_data_size this also might simplify existing code by moving allocation of priv_data to a common place
This doesn't solve the problem of fields that should be initialized to a non-zero state in protocol_open(). We really need an API where we have: 1) a function that allocates both URLContext as well as priv_data context (so the type of protocol, e.g. HTTP, is known at this point) 2) a function that initializes these values to a default state; it makes sense that this function is called at the same time as 1. It also might make sense that the URI is known at this point 3) a window here where we can make changes to this context by calling functions or eventually by introducing AVOptions
AVOptions have a field for defaults thus as soon as a struct can be accssed through AVOptions there also are known defaults
Additionally, as far as I've understood AVOptions, they can only set something within one struct, since it simply uses offsets. For this case, we'd need AVOptions that would only apply if the URLContext happens to be a HTTP context, and then set the options within the priv_data, which can't be done as a simple offset (unless the priv_data is tacked on at the end of the context...)
I think we should check the past discussion about using AVOptions and private contexts. but from memory Adding an AVClass pointer to URLProtocol (and a priv_data_size of course too) allows us to allocate and init to defaults the private context in generic code [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Good people do not need laws to tell them to act responsibly, while bad people will find a way around the laws. -- Plato
On Wed, 16 Jun 2010, Michael Niedermayer wrote:
On Mon, Jun 14, 2010 at 08:12:30PM +0300, Martin Storsjö wrote:
Additionally, as far as I've understood AVOptions, they can only set something within one struct, since it simply uses offsets. For this case, we'd need AVOptions that would only apply if the URLContext happens to be a HTTP context, and then set the options within the priv_data, which can't be done as a simple offset (unless the priv_data is tacked on at the end of the context...)
I think we should check the past discussion about using AVOptions and private contexts. but from memory Adding an AVClass pointer to URLProtocol (and a priv_data_size of course too) allows us to allocate and init to defaults the private context in generic code
Not as far as I understand it. In libavcodec/opt.c, for example av_set_number2, lines 63-70, store values using this logic: dst= ((uint8_t*)obj) + o->offset; *(int*)dst= llrint(num/den)*intnum; So the only things it can express are storing values with a plain offset from the start of the object. There's no code for dereferencing the priv_data pointer and using that as an object. In order to set values within the private data, we'd need to make the private data an AVClass so that it can have different options depending on what the actual protocol is. Then in the generic AVClass for URLContext, we'd need to store the offset of the private data pointer, so that the generic AVOption code can check for options in the private data if the option wasn't found in the generic URLContext. // Martin
On Wed, Jun 16, 2010 at 07:51:32PM +0300, Martin Storsjö wrote:
On Wed, 16 Jun 2010, Michael Niedermayer wrote:
On Mon, Jun 14, 2010 at 08:12:30PM +0300, Martin Storsjö wrote:
Additionally, as far as I've understood AVOptions, they can only set something within one struct, since it simply uses offsets. For this case, we'd need AVOptions that would only apply if the URLContext happens to be a HTTP context, and then set the options within the priv_data, which can't be done as a simple offset (unless the priv_data is tacked on at the end of the context...)
I think we should check the past discussion about using AVOptions and private contexts. but from memory Adding an AVClass pointer to URLProtocol (and a priv_data_size of course too) allows us to allocate and init to defaults the private context in generic code
Not as far as I understand it. In libavcodec/opt.c, for example av_set_number2, lines 63-70, store values using this logic:
dst= ((uint8_t*)obj) + o->offset; *(int*)dst= llrint(num/den)*intnum;
So the only things it can express are storing values with a plain offset from the start of the object. There's no code for dereferencing the priv_data pointer and using that as an object.
In order to set values within the private data, we'd need to make the private data an AVClass so that it can have different options depending on what the actual protocol is. Then in the generic AVClass for URLContext, we'd need to store the offset of the private data pointer, so that the generic AVOption code can check for options in the private data if the option wasn't found in the generic URLContext.
The user code that calls av_set_number2() can just check if it failed and callit on priv_data not saying this is better, iam just saying that the extension you hint at is not strictly needed [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User questions about the command line tools should be sent to the ffmpeg-user ML. And questions about how to use libav* should be sent to the libav-user ML.
On Wed, 16 Jun 2010, Michael Niedermayer wrote:
On Wed, Jun 16, 2010 at 07:51:32PM +0300, Martin Storsjö wrote:
On Wed, 16 Jun 2010, Michael Niedermayer wrote:
On Mon, Jun 14, 2010 at 08:12:30PM +0300, Martin Storsjö wrote:
Additionally, as far as I've understood AVOptions, they can only set something within one struct, since it simply uses offsets. For this case, we'd need AVOptions that would only apply if the URLContext happens to be a HTTP context, and then set the options within the priv_data, which can't be done as a simple offset (unless the priv_data is tacked on at the end of the context...)
I think we should check the past discussion about using AVOptions and private contexts. but from memory Adding an AVClass pointer to URLProtocol (and a priv_data_size of course too) allows us to allocate and init to defaults the private context in generic code
Not as far as I understand it. In libavcodec/opt.c, for example av_set_number2, lines 63-70, store values using this logic:
dst= ((uint8_t*)obj) + o->offset; *(int*)dst= llrint(num/den)*intnum;
So the only things it can express are storing values with a plain offset from the start of the object. There's no code for dereferencing the priv_data pointer and using that as an object.
In order to set values within the private data, we'd need to make the private data an AVClass so that it can have different options depending on what the actual protocol is. Then in the generic AVClass for URLContext, we'd need to store the offset of the private data pointer, so that the generic AVOption code can check for options in the private data if the option wasn't found in the generic URLContext.
The user code that calls av_set_number2() can just check if it failed and callit on priv_data not saying this is better, iam just saying that the extension you hint at is not strictly needed
Hmm, true, that would perhaps simplify things a bit. But even then, we'd need a way to know whether priv_data actually contains an AVClass or not. Either we'd mandate such a member for all protocols' priv_data, or add some flag somewhere in URLContext, e.g. priv_data_has_class. // Martin
On Wed, Jun 16, 2010 at 10:47:03PM +0300, Martin Storsjö wrote:
On Wed, 16 Jun 2010, Michael Niedermayer wrote:
On Wed, Jun 16, 2010 at 07:51:32PM +0300, Martin Storsjö wrote:
On Wed, 16 Jun 2010, Michael Niedermayer wrote:
On Mon, Jun 14, 2010 at 08:12:30PM +0300, Martin Storsjö wrote:
Additionally, as far as I've understood AVOptions, they can only set something within one struct, since it simply uses offsets. For this case, we'd need AVOptions that would only apply if the URLContext happens to be a HTTP context, and then set the options within the priv_data, which can't be done as a simple offset (unless the priv_data is tacked on at the end of the context...)
I think we should check the past discussion about using AVOptions and private contexts. but from memory Adding an AVClass pointer to URLProtocol (and a priv_data_size of course too) allows us to allocate and init to defaults the private context in generic code
Not as far as I understand it. In libavcodec/opt.c, for example av_set_number2, lines 63-70, store values using this logic:
dst= ((uint8_t*)obj) + o->offset; *(int*)dst= llrint(num/den)*intnum;
So the only things it can express are storing values with a plain offset from the start of the object. There's no code for dereferencing the priv_data pointer and using that as an object.
In order to set values within the private data, we'd need to make the private data an AVClass so that it can have different options depending on what the actual protocol is. Then in the generic AVClass for URLContext, we'd need to store the offset of the private data pointer, so that the generic AVOption code can check for options in the private data if the option wasn't found in the generic URLContext.
The user code that calls av_set_number2() can just check if it failed and callit on priv_data not saying this is better, iam just saying that the extension you hint at is not strictly needed
Hmm, true, that would perhaps simplify things a bit. But even then, we'd need a way to know whether priv_data actually contains an AVClass or not. Either we'd mandate such a member for all protocols' priv_data, or add some flag somewhere in URLContext, e.g. priv_data_has_class.
id mandate it but allow it to be NULL [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Its not that you shouldnt use gotos but rather that you should write readable code and code with gotos often but not always is less readable
On Wed, 16 Jun 2010, Michael Niedermayer wrote:
On Wed, Jun 16, 2010 at 10:47:03PM +0300, Martin Storsjö wrote:
On Wed, 16 Jun 2010, Michael Niedermayer wrote:
On Wed, Jun 16, 2010 at 07:51:32PM +0300, Martin Storsjö wrote:
On Wed, 16 Jun 2010, Michael Niedermayer wrote:
On Mon, Jun 14, 2010 at 08:12:30PM +0300, Martin Storsjö wrote:
Additionally, as far as I've understood AVOptions, they can only set something within one struct, since it simply uses offsets. For this case, we'd need AVOptions that would only apply if the URLContext happens to be a HTTP context, and then set the options within the priv_data, which can't be done as a simple offset (unless the priv_data is tacked on at the end of the context...)
I think we should check the past discussion about using AVOptions and private contexts. but from memory Adding an AVClass pointer to URLProtocol (and a priv_data_size of course too) allows us to allocate and init to defaults the private context in generic code
Not as far as I understand it. In libavcodec/opt.c, for example av_set_number2, lines 63-70, store values using this logic:
dst= ((uint8_t*)obj) + o->offset; *(int*)dst= llrint(num/den)*intnum;
So the only things it can express are storing values with a plain offset from the start of the object. There's no code for dereferencing the priv_data pointer and using that as an object.
In order to set values within the private data, we'd need to make the private data an AVClass so that it can have different options depending on what the actual protocol is. Then in the generic AVClass for URLContext, we'd need to store the offset of the private data pointer, so that the generic AVOption code can check for options in the private data if the option wasn't found in the generic URLContext.
The user code that calls av_set_number2() can just check if it failed and callit on priv_data not saying this is better, iam just saying that the extension you hint at is not strictly needed
Hmm, true, that would perhaps simplify things a bit. But even then, we'd need a way to know whether priv_data actually contains an AVClass or not. Either we'd mandate such a member for all protocols' priv_data, or add some flag somewhere in URLContext, e.g. priv_data_has_class.
id mandate it but allow it to be NULL
Thanks, then it should be quite straightforward to implement. What modifications to URLProtocol are allowed without requiring a major bump? Since we've got a public function av_register_protocol that takes an URLProtocol*, doesn't even a simple addition of new fields require a major bump, since external apps could register their own URLProtocols? Despite this, the last lavf major bump was in November 2007, but three new fields have been added to URLProtocol since that. If not, the rest of this should be quite easy. // Martin
On Sun, Jun 20, 2010 at 07:35:40PM +0300, Martin Storsjö wrote:
On Wed, 16 Jun 2010, Michael Niedermayer wrote:
On Wed, Jun 16, 2010 at 10:47:03PM +0300, Martin Storsjö wrote:
On Wed, 16 Jun 2010, Michael Niedermayer wrote:
On Wed, Jun 16, 2010 at 07:51:32PM +0300, Martin Storsjö wrote:
On Wed, 16 Jun 2010, Michael Niedermayer wrote:
On Mon, Jun 14, 2010 at 08:12:30PM +0300, Martin Storsjö wrote: > > Additionally, as far as I've understood AVOptions, they can only set > something within one struct, since it simply uses offsets. For this case, > we'd need AVOptions that would only apply if the URLContext happens to be > a HTTP context, and then set the options within the priv_data, which can't > be done as a simple offset (unless the priv_data is tacked on at the > end of the context...)
I think we should check the past discussion about using AVOptions and private contexts. but from memory Adding an AVClass pointer to URLProtocol (and a priv_data_size of course too) allows us to allocate and init to defaults the private context in generic code
Not as far as I understand it. In libavcodec/opt.c, for example av_set_number2, lines 63-70, store values using this logic:
dst= ((uint8_t*)obj) + o->offset; *(int*)dst= llrint(num/den)*intnum;
So the only things it can express are storing values with a plain offset from the start of the object. There's no code for dereferencing the priv_data pointer and using that as an object.
In order to set values within the private data, we'd need to make the private data an AVClass so that it can have different options depending on what the actual protocol is. Then in the generic AVClass for URLContext, we'd need to store the offset of the private data pointer, so that the generic AVOption code can check for options in the private data if the option wasn't found in the generic URLContext.
The user code that calls av_set_number2() can just check if it failed and callit on priv_data not saying this is better, iam just saying that the extension you hint at is not strictly needed
Hmm, true, that would perhaps simplify things a bit. But even then, we'd need a way to know whether priv_data actually contains an AVClass or not. Either we'd mandate such a member for all protocols' priv_data, or add some flag somewhere in URLContext, e.g. priv_data_has_class.
id mandate it but allow it to be NULL
Thanks, then it should be quite straightforward to implement.
What modifications to URLProtocol are allowed without requiring a major bump? Since we've got a public function av_register_protocol that takes an URLProtocol*, doesn't even a simple addition of new fields require a major bump, since external apps could register their own URLProtocols? Despite
uhm hmm :( a hair of hackery that would solve this is to set next=1 on all of ours and malloc() + copy if its not so in register under #if [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Old school: Use the lowest level language in which you can solve the problem conveniently. New school: Use the highest level language in which the latest supercomputer can solve the problem without the user falling asleep waiting.
On Mon, 21 Jun 2010, Michael Niedermayer wrote:
On Sun, Jun 20, 2010 at 07:35:40PM +0300, Martin Storsjö wrote:
What modifications to URLProtocol are allowed without requiring a major bump? Since we've got a public function av_register_protocol that takes an URLProtocol*, doesn't even a simple addition of new fields require a major bump, since external apps could register their own URLProtocols? Despite
uhm hmm :( a hair of hackery that would solve this is to set next=1 on all of ours and malloc() + copy if its not so in register under #if
That's one possibility, although needing to set next=1 on all of ours sure feels a bit hacky. Another solution would be to add an av_register_protocol2, that takes a size parameter. Then all code that uses it can do av_register_protocol2(&myproto, sizeof(myproto)), and our code can use it sensibly. That would be more flexible if we need to add even more fields later. If we add fields to our URLProtocol, the external code will start using a malloc, until they recompile it with the latest headers again. I've got a first version of this patch series ready now, I'll post it to -devel. // Martin
On 06/16/2010 06:19 PM, Michael Niedermayer wrote:
AVOptions have a field for defaults thus as soon as a struct can be accssed through AVOptions there also are known defaults
I'm not sure how that would bode since rfc822 headers could be quite various depending on the protocols, right now we do not care about speed when we read/store/overwrite so the method is fine. AVOption seems fast but lacking in flexibility. That's why even if overkill I preferred a dict as struct to store/manage them. lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On Mon, 7 Jun 2010, Josh Allmann wrote:
Luca, Martin, any more comments?
#3 seemed ok, no comments, applied. The lack of a #4 in this series indicates that you hadn't rebased this on top of the latest master/trunk. ... which bit me when I tried to apply #5, which didn't apply cleanly. So make sure you rebase on top of the latest versions regularly, especially if there are other changes to the areas you're working on! Except for that, it seemed ok, so applied. I added an error message in ff_rtsp_send_cmd_with_content_async if you try to send body data with the request in tunneling mode, since the body data needs to be base64 encoded together with the request. Currently, only the RTSP muxer does such requests (for which the HTTP tunneling is disabled), but I added it to make sure noone runs into problems if someone adds such requests to the RTSP demuxer. Despite all the other feedback, congrats on getting the first major feature committed. // Martin
Hi, On Wed, Jun 2, 2010 at 6:59 PM, Josh Allmann <joshua.allmann@gmail.com> wrote:
First three are related to http, the last adds actual tunnelling support to rtsp.
HTTP doesn't need the new flag, you can always open delayed. For RTSP/HTTP.
+ /* base64 encode rtsp if tunnelling */ + if (rt->rtsp_hd_out != rt->rtsp_hd) { + if (!av_base64_encode(base64buf, sizeof(base64buf), buf, strlen(buf))) { + av_log(s, AV_LOG_ERROR, "Unable to base64 encode RTSP.\n"); + return; + } + out_buf = base64buf; + } +
I don't like the if at the top, I'd add a rtsp_protocol_mode with an enum (like the transport enums in rtsp.h), using RTSP_PROTOCOL_MODE_PLAIN or RTSP_PROTOCOL_MODE_HTTP. You can also call it tunnel_mode or so.
- url_write(rt->rtsp_hd, buf, strlen(buf)); + url_write(rt->rtsp_hd_out, out_buf, strlen(out_buf)); if (send_content_length > 0 && send_content) - url_write(rt->rtsp_hd, send_content, send_content_length); + url_write(rt->rtsp_hd_out, send_content, send_content_length);
I'd prefer if you'd do the patch that splits rtsp_hd into hd_in/out separately. + "Content-Length: 32767\r\n" + "Expires: Sun, 9 Jan 1972 00:00:00 GMT\r\n", ?? The rest is pretty, well done! Ronald
Five patches in this series, addressing comments from everyone. My soc repo is also updated. I've pulled against the latest ffmpeg trunk, so we can use the av_base64_size macro and the interleaving fix Martin made. http://github.com/j0sh/ffmpeg-soc/tree/rtsp-tunneling On 3 June 2010 11:51, Ronald S. Bultje <rsbultje@gmail.com> wrote:
Hi,
On Wed, Jun 2, 2010 at 6:59 PM, Josh Allmann <joshua.allmann@gmail.com> wrote:
First three are related to http, the last adds actual tunnelling support to rtsp.
HTTP doesn't need the new flag, you can always open delayed.
Done. Also, after talking with Martin about this on IRC, I also special-cased the size parameter to force a header fetch. However, I can just add a separate ff_http_connect function along the lines of my first patchset if that's preferable.
For RTSP/HTTP.
+ /* base64 encode rtsp if tunnelling */ + if (rt->rtsp_hd_out != rt->rtsp_hd) { + if (!av_base64_encode(base64buf, sizeof(base64buf), buf, strlen(buf))) { + av_log(s, AV_LOG_ERROR, "Unable to base64 encode RTSP.\n"); + return; + } + out_buf = base64buf; + } +
I don't like the if at the top, I'd add a rtsp_protocol_mode with an enum (like the transport enums in rtsp.h), using RTSP_PROTOCOL_MODE_PLAIN or RTSP_PROTOCOL_MODE_HTTP. You can also call it tunnel_mode or so.
Done.
- url_write(rt->rtsp_hd, buf, strlen(buf)); + url_write(rt->rtsp_hd_out, out_buf, strlen(out_buf)); if (send_content_length > 0 && send_content) - url_write(rt->rtsp_hd, send_content, send_content_length); + url_write(rt->rtsp_hd_out, send_content, send_content_length);
I'd prefer if you'd do the patch that splits rtsp_hd into hd_in/out separately.
+ "Content-Length: 32767\r\n" + "Expires: Sun, 9 Jan 1972 00:00:00 GMT\r\n",
??
So quoth the spec. They're there to prevent proxies from caching stuff.
The rest is pretty, well done!
Thanks!
participants (5)
-
Josh Allmann -
Luca Barbato -
Martin Storsjö -
Michael Niedermayer -
Ronald S. Bultje