Re: [FFmpeg-soc] [PATCH] decouple mpeg4/aac from rtsp
Sent to luca, but not ffmpeg-soc... On 22 June 2010 23:52, Josh Allmann <joshua.allmann@gmail.com> wrote:
Hi,
On 21 June 2010 14:28, Luca Barbato <lu_zero@gentoo.org> wrote:
On 21/06/2010 21:18, Josh Allmann wrote:
Okay, updated patches attached, with more forthcoming. (Ignore the numbering; that teaches me to do git rebase instead of git pull...)
0001 should use ff_ namespace probably.
Done
0029 not sure if hex_to_data might have other usages and thus should be put in the header as well.
Left as-is per Ronald's suggestion.
The rest seems ok
Three more patches attached which clean up the rest.
004 breaks receiving video (mpeg4 AND h264) if streamed in conjunction with AAC, but video seems to work OK in isolation. Which is weird because 004 should only touch the AAC codepath.
Been staring at this bug all day, so I'm just gonna turn in what I have, get some sleep, and hopefully dream up a fix.
Josh
On Tue, Jun 22, 2010 at 11:55:01PM -0700, Josh Allmann wrote:
Sent to luca, but not ffmpeg-soc...
On 22 June 2010 23:52, Josh Allmann <joshua.allmann@gmail.com> wrote:
Hi,
On 21 June 2010 14:28, Luca Barbato <lu_zero@gentoo.org> wrote:
On 21/06/2010 21:18, Josh Allmann wrote:
Okay, updated patches attached, with more forthcoming. (Ignore the numbering; that teaches me to do git rebase instead of git pull...)
0001 should use ff_ namespace probably.
Done
0029 not sure if hex_to_data might have other usages and thus should be put in the header as well.
Left as-is per Ronald's suggestion.
The rest seems ok
Three more patches attached which clean up the rest.
004 breaks receiving video (mpeg4 AND h264) if streamed in conjunction with AAC, but video seems to work OK in isolation. Which is weird because 004 should only touch the AAC codepath.
Been staring at this bug all day, so I'm just gonna turn in what I have, get some sleep, and hopefully dream up a fix.
Josh
internal.h | 13 +++++++++++++ rtsp.c | 35 +++++++++++------------------------ 2 files changed, 24 insertions(+), 24 deletions(-) ebf55dc8176bc456ad4d776532dd1a78edba726f 0001-Move-skip_spaces-to-libavformat-internal.h.patch From 52d6caf62b08a7591ba5459a5966b7f7b0f8b23f Mon Sep 17 00:00:00 2001 From: Josh Allmann <joshua.allmann@gmail.com> Date: Sun, 20 Jun 2010 12:12:22 -0700 Subject: [PATCH 1/5] Move skip_spaces to libavformat/internal.h
--- libavformat/internal.h | 13 +++++++++++++ libavformat/rtsp.c | 35 +++++++++++------------------------ 2 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/libavformat/internal.h b/libavformat/internal.h index 4489ffe..aaa71ac 100644 --- a/libavformat/internal.h +++ b/libavformat/internal.h @@ -174,4 +174,17 @@ void ff_sdp_write_media(char *buff, int size, AVCodecContext *c, int ff_write_chained(AVFormatContext *dst, int dst_stream, AVPacket *pkt, AVFormatContext *src);
+#define SPACE_CHARS " \t\r\n" +/* we use memchr() instead of strchr() here because strchr() will return + * the terminating '\0' of SPACE_CHARS instead of NULL if c is '\0'. */ +#define redir_isspace(c) memchr(SPACE_CHARS, c, 4)
strspn ? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I hate to see young programmers poisoned by the kind of thinking Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
Hi, On Wed, Jun 23, 2010 at 6:13 AM, Michael Niedermayer <michaelni@gmx.at> wrote:
On Tue, Jun 22, 2010 at 11:55:01PM -0700, Josh Allmann wrote:
Sent to luca, but not ffmpeg-soc... On 22 June 2010 23:52, Josh Allmann <joshua.allmann@gmail.com> wrote:
On 21 June 2010 14:28, Luca Barbato <lu_zero@gentoo.org> wrote:
On 21/06/2010 21:18, Josh Allmann wrote:
Okay, updated patches attached, with more forthcoming. (Ignore the numbering; that teaches me to do git rebase instead of git pull...)
0001 should use ff_ namespace probably.
Done
0029 not sure if hex_to_data might have other usages and thus should be put in the header as well.
Left as-is per Ronald's suggestion.
The rest seems ok
Three more patches attached which clean up the rest.
004 breaks receiving video (mpeg4 AND h264) if streamed in conjunction with AAC, but video seems to work OK in isolation. Which is weird because 004 should only touch the AAC codepath.
Been staring at this bug all day, so I'm just gonna turn in what I have, get some sleep, and hopefully dream up a fix.
Josh
internal.h | 13 +++++++++++++ rtsp.c | 35 +++++++++++------------------------ 2 files changed, 24 insertions(+), 24 deletions(-) ebf55dc8176bc456ad4d776532dd1a78edba726f 0001-Move-skip_spaces-to-libavformat-internal.h.patch From 52d6caf62b08a7591ba5459a5966b7f7b0f8b23f Mon Sep 17 00:00:00 2001 From: Josh Allmann <joshua.allmann@gmail.com> Date: Sun, 20 Jun 2010 12:12:22 -0700 Subject: [PATCH 1/5] Move skip_spaces to libavformat/internal.h
--- libavformat/internal.h | 13 +++++++++++++ libavformat/rtsp.c | 35 +++++++++++------------------------ 2 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/libavformat/internal.h b/libavformat/internal.h index 4489ffe..aaa71ac 100644 --- a/libavformat/internal.h +++ b/libavformat/internal.h @@ -174,4 +174,17 @@ void ff_sdp_write_media(char *buff, int size, AVCodecContext *c, int ff_write_chained(AVFormatContext *dst, int dst_stream, AVPacket *pkt, AVFormatContext *src);
+#define SPACE_CHARS " \t\r\n" +/* we use memchr() instead of strchr() here because strchr() will return + * the terminating '\0' of SPACE_CHARS instead of NULL if c is '\0'. */ +#define redir_isspace(c) memchr(SPACE_CHARS, c, 4)
strspn ?
strspn() is interesting, does not appear to have the bug that strchr has (causing us to use memchr). I'm fine with switching. Ronald
Hi Josh, On Tue, 22 Jun 2010, Josh Allmann wrote:
004 breaks receiving video (mpeg4 AND h264) if streamed in conjunction with AAC, but video seems to work OK in isolation. Which is weird because 004 should only touch the AAC codepath.
Been staring at this bug all day, so I'm just gonna turn in what I have, get some sleep, and hopefully dream up a fix.
Whoa, this is good stuff. Can't wait to commit it :-) I found the missing thing that messes up streams with both audio and video - in the code branch in rtpdec.c for formats without a parse_packet function, the generic code sets pkt->stream_index = st->index, which you need to do in your dynamic payload handler. Except for that, patch #5 had some issues where you had forgotten to rename infos to data - I guess you've missed to commit something here? In the same patch, you've also got a case where you do if (ptr) av_free(ptr), which is't necessary, av_free() can handle that itself. Also, you don't need any new_context/free_context for mp4v_es, since it doesn't use that context. In order to do that, you need to make sure that you won't accidentally use the PayloadContext in parse_sdp_line unless the codec is AAC. In free_context, you should move the opening brace of the for loop. :-) I'm not sure about the copyright owner of this file, that you've set to "the FFmpeg developers" - is it possible to dig through the version history to see who wrote this initially? Apart from these details - I really like this, good work so far! // Martin
Hi, On 23 June 2010 09:02, Martin Storsjö <martin@martin.st> wrote:
Hi Josh,
On Tue, 22 Jun 2010, Josh Allmann wrote:
004 breaks receiving video (mpeg4 AND h264) if streamed in conjunction with AAC, but video seems to work OK in isolation. Which is weird because 004 should only touch the AAC codepath.
Been staring at this bug all day, so I'm just gonna turn in what I have, get some sleep, and hopefully dream up a fix.
Whoa, this is good stuff. Can't wait to commit it :-)
I found the missing thing that messes up streams with both audio and video - in the code branch in rtpdec.c for formats without a parse_packet function, the generic code sets pkt->stream_index = st->index, which you need to do in your dynamic payload handler.
Yep, thanks for the second pair of eyes...
Except for that, patch #5 had some issues where you had forgotten to rename infos to data - I guess you've missed to commit something here?
Yeah, sorry for the broken commits. Fixed, but I tried to keep the diffs as small as possible, so I kept infos (the original name) did another patch (007) that changes infos to data, to maintain consistency with the rest of rtpdec*.
In the same patch, you've also got a case where you do if (ptr) av_free(ptr), which is't necessary, av_free() can handle that itself.
Fixed
Also, you don't need any new_context/free_context for mp4v_es, since it doesn't use that context. In order to do that, you need to make sure that you won't accidentally use the PayloadContext in parse_sdp_line unless the codec is AAC.
Fixed
In free_context, you should move the opening brace of the for loop. :-)
Fixed
I'm not sure about the copyright owner of this file, that you've set to "the FFmpeg developers" - is it possible to dig through the version history to see who wrote this initially?
Fixed -- Fabrice Bellard added in MPEG-4 video support (r1223), and Romain Degez did AAC (r4306). I set the copyright to 2010, though.
Apart from these details - I really like this, good work so far!
Awesome -- patch 002 in this series also replaces memchr with strspn, as noted by Michael and Ronald. Josh
On Wed, Jun 23, 2010 at 03:57:20PM -0700, Josh Allmann wrote:
Hi,
On 23 June 2010 14:48, Josh Allmann <joshua.allmann@gmail.com> wrote:
Awesome -- patch 002 in this series also replaces memchr with strspn, as noted by Michael and Ronald.
Updated version attached.
Josh
internal.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 6e0a821a7df48a080706183c1bf3e7483986e1af 0002-Use-strspn-rather-than-memchr-in-ff_skip_spaces.patch From abd99d7f37708fa6c41825eb4a2c37133f7d62f5 Mon Sep 17 00:00:00 2001 From: Josh Allmann <joshua.allmann@gmail.com> Date: Wed, 23 Jun 2010 14:29:27 -0700 Subject: [PATCH 2/7] Use strspn rather than memchr in ff_skip_spaces.
--- libavformat/internal.h | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/libavformat/internal.h b/libavformat/internal.h index aaa71ac..ebe5d6b 100644 --- a/libavformat/internal.h +++ b/libavformat/internal.h @@ -175,15 +175,17 @@ int ff_write_chained(AVFormatContext *dst, int dst_stream, AVPacket *pkt, AVFormatContext *src);
#define SPACE_CHARS " \t\r\n" -/* we use memchr() instead of strchr() here because strchr() will return +/* we use strspn() instead of strchr() here because strchr() will return * the terminating '\0' of SPACE_CHARS instead of NULL if c is '\0'. */ -#define redir_isspace(c) memchr(SPACE_CHARS, c, 4) +#define redir_isspace(c) strspn(c, SPACE_CHARS) static inline void ff_skip_spaces(const char **pp) { const char *p; + int span; + p = *pp; - while (redir_isspace(*p)) - p++; + span = redir_isspace(p); + p += span; *pp = p; }
you can call strspn(SPACE_CHARS) directly, you dont need ff_skip_spaces() [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you really think that XML is the answer, then you definitly missunderstood the question -- Attila Kinali
On 23 June 2010 17:04, Michael Niedermayer <michaelni@gmx.at> wrote:
On Wed, Jun 23, 2010 at 03:57:20PM -0700, Josh Allmann wrote:
Hi,
On 23 June 2010 14:48, Josh Allmann <joshua.allmann@gmail.com> wrote:
Awesome -- patch 002 in this series also replaces memchr with strspn, as noted by Michael and Ronald.
Updated version attached.
Josh
you can call strspn(SPACE_CHARS) directly, you dont need ff_skip_spaces()
Ohh, nice one. Fixed in both patches that contain ff_skip_spaces. Josh
On Wed, 23 Jun 2010, Josh Allmann wrote:
Awesome -- patch 002 in this series also replaces memchr with strspn, as noted by Michael and Ronald.
This looks good to me (using #1 and #2 from the later mail). When moving sdp_parse_fmtp_config from rtsp.c to to be called parse_sdp_line in rtpdec_mpeg4, you could reindent the second parameter line right there instead of doing it in patch #7. Except that, this looks good to me. Ronald, Luca, any other opinions, or is this good to go? // Martin
On 24 June 2010 00:14, Martin Storsjö <martin@martin.st> wrote:
On Wed, 23 Jun 2010, Josh Allmann wrote:
Awesome -- patch 002 in this series also replaces memchr with strspn, as noted by Michael and Ronald.
This looks good to me (using #1 and #2 from the later mail). When moving sdp_parse_fmtp_config from rtsp.c to to be called parse_sdp_line in rtpdec_mpeg4, you could reindent the second parameter line right there instead of doing it in patch #7.
Fixed.
Except that, this looks good to me. Ronald, Luca, any other opinions, or is this good to go?
Sweeet. Updated patchset is properly numbered.
On 06/24/2010 10:31 AM, Josh Allmann wrote:
On 24 June 2010 00:14, Martin Storsjö <martin@martin.st> wrote:
On Wed, 23 Jun 2010, Josh Allmann wrote:
Awesome -- patch 002 in this series also replaces memchr with strspn, as noted by Michael and Ronald.
This looks good to me (using #1 and #2 from the later mail). When moving sdp_parse_fmtp_config from rtsp.c to to be called parse_sdp_line in rtpdec_mpeg4, you could reindent the second parameter line right there instead of doing it in patch #7.
Fixed.
Except that, this looks good to me. Ronald, Luca, any other opinions, or is this good to go?
Sweeet. Updated patchset is properly numbered.
Two nits 0002 + * MPEG-4 Video RTP callbacks. Might be nice adding the rfc number there as well 0004 +#include <strings.h> string_s_ ? Beside that who tested them? lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On 24 June 2010 12:53, Luca Barbato <lu_zero@gentoo.org> wrote:
On 06/24/2010 10:31 AM, Josh Allmann wrote:
On 24 June 2010 00:14, Martin Storsjö <martin@martin.st> wrote:
Except that, this looks good to me. Ronald, Luca, any other opinions, or is this good to go?
Sweeet. Updated patchset is properly numbered.
Two nits
0002
+ * MPEG-4 Video RTP callbacks.
Might be nice adding the rfc number there as well
Added reference to RFC 3016.
0004
+#include <strings.h>
string_s_ ?
Needed for strcasecmp, apparently. Also fixed a related issue; strings.h is now included in patch 003 rather than 004. Josh
On 24 June 2010 13:46, Josh Allmann <joshua.allmann@gmail.com> wrote:
On 24 June 2010 12:53, Luca Barbato <lu_zero@gentoo.org> wrote:
On 06/24/2010 10:31 AM, Josh Allmann wrote:
On 24 June 2010 00:14, Martin Storsjö <martin@martin.st> wrote:
Except that, this looks good to me. Ronald, Luca, any other opinions, or is this good to go?
Sweeet. Updated patchset is properly numbered.
Two nits
0002
+ * MPEG-4 Video RTP callbacks.
Might be nice adding the rfc number there as well
Added reference to RFC 3016.
0004
+#include <strings.h>
string_s_ ?
Needed for strcasecmp, apparently. Also fixed a related issue; strings.h is now included in patch 003 rather than 004.
The minor indentation change Martin asked for in 002 somehow disappeared; patch re-done with this.
Josh
On 24 June 2010 14:01, Josh Allmann <joshua.allmann@gmail.com> wrote:
On 24 June 2010 13:46, Josh Allmann <joshua.allmann@gmail.com> wrote:
On 24 June 2010 12:53, Luca Barbato <lu_zero@gentoo.org> wrote:
On 06/24/2010 10:31 AM, Josh Allmann wrote:
On 24 June 2010 00:14, Martin Storsjö <martin@martin.st> wrote:
Except that, this looks good to me. Ronald, Luca, any other opinions, or is this good to go?
Sweeet. Updated patchset is properly numbered.
Two nits
0002
+ * MPEG-4 Video RTP callbacks.
Might be nice adding the rfc number there as well
Added reference to RFC 3016.
0004
+#include <strings.h>
string_s_ ?
Needed for strcasecmp, apparently. Also fixed a related issue; strings.h is now included in patch 003 rather than 004.
The minor indentation change Martin asked for in 002 somehow disappeared; patch re-done with this.
Re-attaching the whole series. Tested and works OK. Thumbs up??
On Thu, 24 Jun 2010, Josh Allmann wrote:
Re-attaching the whole series. Tested and works OK. Thumbs up??
Looks good to me. Ok to apply, Luca? // Martin
On 06/24/2010 11:40 PM, Martin Storsjö wrote:
On Thu, 24 Jun 2010, Josh Allmann wrote:
Re-attaching the whole series. Tested and works OK. Thumbs up??
Looks good to me. Ok to apply, Luca?
Looks fine. -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On Fri, 25 Jun 2010, Luca Barbato wrote:
On 06/24/2010 11:40 PM, Martin Storsjö wrote:
On Thu, 24 Jun 2010, Josh Allmann wrote:
Re-attaching the whole series. Tested and works OK. Thumbs up??
Looks good to me. Ok to apply, Luca?
Looks fine.
Applied all of them - good job Josh! // Martin
On 06/24/2010 09:14 AM, Martin Storsjö wrote:
On Wed, 23 Jun 2010, Josh Allmann wrote:
Awesome -- patch 002 in this series also replaces memchr with strspn, as noted by Michael and Ronald.
This looks good to me (using #1 and #2 from the later mail). When moving sdp_parse_fmtp_config from rtsp.c to to be called parse_sdp_line in rtpdec_mpeg4, you could reindent the second parameter line right there instead of doing it in patch #7.
Except that, this looks good to me. Ronald, Luca, any other opinions, or is this good to go?
Seems so, I had just a look over them, if there isn't rush I'll try them in the evening. lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
Hi, On Thu, Jun 24, 2010 at 3:14 AM, Martin Storsjö <martin@martin.st> wrote:
Except that, this looks good to me. Ronald, Luca, any other opinions, or is this good to go?
Latest looks good to me, great work Josh! Ronald
participants (5)
-
Josh Allmann -
Luca Barbato -
Martin Storsjö -
Michael Niedermayer -
Ronald S. Bultje