Message ID | 1305893640-56569-9-git-send-email-martin@martin.st |
---|---|
State | Superseded |
Headers | show |
Hi, On Fri, May 20, 2011 at 8:14 AM, Martin Storsjö <martin@martin.st> wrote: > This has a different name ("rtphinting" vs "rtphint"), since > the same name can't be used, even though it formerly was used > as a flag (-fflags rtphint) vs straight AVOption now > (-rtphinting 1). > --- > libavformat/avformat.h | 2 +- > libavformat/movenc.c | 8 +++++++- > libavformat/movenc.h | 1 + > libavformat/options.c | 2 +- > 4 files changed, 10 insertions(+), 3 deletions(-) This patch is fine regardless of the others. Ronald
On 20/05/2011, at 14.14, Martin Storsjö wrote: > This has a different name ("rtphinting" vs "rtphint"), since > the same name can't be used, even though it formerly was used > as a flag (-fflags rtphint) vs straight AVOption now > (-rtphinting 1). As a sidenote, this patch also seems to completely deprecate rtp hinting from the 3GP2, iPod & PSP muxers. I think the 3GP2 muxer should still be able to hint, or am I wrong? Regards, Gil
On Sat, 21 May 2011, Gil Pedersen wrote: > On 20/05/2011, at 14.14, Martin Storsjö wrote: > > > This has a different name ("rtphinting" vs "rtphint"), since > > the same name can't be used, even though it formerly was used > > as a flag (-fflags rtphint) vs straight AVOption now > > (-rtphinting 1). > > As a sidenote, this patch also seems to completely deprecate rtp hinting > from the 3GP2, iPod & PSP muxers. I think the 3GP2 muxer should still be > able to hint, or am I wrong? Sure, if you think it's useful to hint 3gp2, it's no problem to add it to that one, too, or to all of them. I kept it short since I thought that were the only ones that this mattered for. // Martin
On 21/05/2011, at 12.58, "Martin Storsjö" <martin@martin.st> wrote: > On Sat, 21 May 2011, Gil Pedersen wrote: > >> On 20/05/2011, at 14.14, Martin Storsjö wrote: >> >>> This has a different name ("rtphinting" vs "rtphint"), since >>> the same name can't be used, even though it formerly was used >>> as a flag (-fflags rtphint) vs straight AVOption now >>> (-rtphinting 1). >> >> As a sidenote, this patch also seems to completely deprecate rtp hinting >> from the 3GP2, iPod & PSP muxers. I think the 3GP2 muxer should still be >> able to hint, or am I wrong? > > Sure, if you think it's useful to hint 3gp2, it's no problem to add it to > that one, too, or to all of them. I kept it short since I thought that > were the only ones that this mattered for. RTP hinting is just as valid for 3gp2 as the other formats. It seems wrong to hint iPod and PSP files and I guess you can use an external tool in those cases, like when you want to prepare mov/mp4/3gp files for progressive download. The RTP hinting done by libav is quite limited, as it is, and you're already likely to use mp4box, or similar tool, for any non-basic hinting. /Gil
On Sun, 22 May 2011, Gil Pedersen wrote: > On 21/05/2011, at 12.58, "Martin Storsjö" <martin@martin.st> wrote: > > > On Sat, 21 May 2011, Gil Pedersen wrote: > > > >> On 20/05/2011, at 14.14, Martin Storsjö wrote: > >> > >>> This has a different name ("rtphinting" vs "rtphint"), since > >>> the same name can't be used, even though it formerly was used > >>> as a flag (-fflags rtphint) vs straight AVOption now > >>> (-rtphinting 1). > >> > >> As a sidenote, this patch also seems to completely deprecate rtp hinting > >> from the 3GP2, iPod & PSP muxers. I think the 3GP2 muxer should still be > >> able to hint, or am I wrong? > > > > Sure, if you think it's useful to hint 3gp2, it's no problem to add it to > > that one, too, or to all of them. I kept it short since I thought that > > were the only ones that this mattered for. > > RTP hinting is just as valid for 3gp2 as the other formats. Sure, that's true. > It seems wrong to hint iPod and PSP files and I guess you can use an > external tool in those cases, like when you want to prepare mov/mp4/3gp > files for progressive download. The RTP hinting done by libav is quite > limited, as it is, and you're already likely to use mp4box, or similar > tool, for any non-basic hinting. Oh, what limitations have you run into, that mp4box handles better? // Martin
On 22/05/2011, at 12.17, "Martin Storsjö" <martin@martin.st> wrote: > On Sun, 22 May 2011, Gil Pedersen wrote: > >> It seems wrong to hint iPod and PSP files and I guess you can use an >> external tool in those cases, like when you want to prepare mov/mp4/3gp >> files for progressive download. The RTP hinting done by libav is quite >> limited, as it is, and you're already likely to use mp4box, or similar >> tool, for any non-basic hinting. > > Oh, what limitations have you run into, that mp4box handles better? It has a whole host of rtp hinting related options, as can be seen on the man page: http://manpages.ubuntu.com/manpages/natty/man1/mp4box.1.html I think the only option libav supports is LATM payload streaming, but I might be wrong. /Gil
On Sun, 22 May 2011, Gil Pedersen wrote: > On 22/05/2011, at 12.17, "Martin Storsjö" <martin@martin.st> wrote: > > > On Sun, 22 May 2011, Gil Pedersen wrote: > > > >> The RTP hinting done by libav is quite limited, as it is, and you're > >> already likely to use mp4box, or similar tool, for any non-basic > >> hinting. > > > > Oh, what limitations have you run into, that mp4box handles better? > > It has a whole host of rtp hinting related options, as can be seen on the man page: > http://manpages.ubuntu.com/manpages/natty/man1/mp4box.1.html > > I think the only option libav supports is LATM payload streaming, but I > might be wrong. Well, comparing a list of options perhaps isn't indicative of the number of features. We do support RTP hinting for all the formats that our RTP muxer supports, which is quite a few. There's just not many public options to control its behaviour, yet at least. I don't really see any critical feature among those options that we are missing. The MTU isn't configurable in our case (adding an AVOption for that is quite easy if someone has a need for it), and we don't support copying hint tracks (I think), adding extra custom SDP data nor choosing whether to prefer dynamic payload IDs over static ones. The number of packets to concatenate into an RTP packet is configurable via max_delay though. If someone has a concrete usecase where some other parameter needs to be adjusted, where the default isn't ok, we can of course try to make that configurable, as we're discussing with the LATM case right now. // Martin
On 22/05/2011, at 12.57, Martin Storsjö wrote: > On Sun, 22 May 2011, Gil Pedersen wrote: > >> On 22/05/2011, at 12.17, "Martin Storsjö" <martin@martin.st> wrote: >> >>> On Sun, 22 May 2011, Gil Pedersen wrote: >>> >>>> The RTP hinting done by libav is quite limited, as it is, and you're >>>> already likely to use mp4box, or similar tool, for any non-basic >>>> hinting. >>> >>> Oh, what limitations have you run into, that mp4box handles better? >> >> It has a whole host of rtp hinting related options, as can be seen on the man page: >> http://manpages.ubuntu.com/manpages/natty/man1/mp4box.1.html >> >> I think the only option libav supports is LATM payload streaming, but I >> might be wrong. > > Well, comparing a list of options perhaps isn't indicative of the number > of features. We do support RTP hinting for all the formats that our RTP > muxer supports, which is quite a few. There's just not many public options > to control its behaviour, yet at least. > > I don't really see any critical feature among those options that we are > missing. The MTU isn't configurable in our case (adding an AVOption for > that is quite easy if someone has a need for it), and we don't support > copying hint tracks (I think), adding extra custom SDP data nor choosing > whether to prefer dynamic payload IDs over static ones. The number of > packets to concatenate into an RTP packet is configurable via max_delay > though. I'm just stating how it looks at the moment, and I agree that several of the mp4box options could be implemented quite easily in libav. Other options, like interleaving and fast start, will require more substantial changes to the movenc muxer since they can't be realized using the current implementation. Ie. you need to create a temporary file and write everything to disk twice for it to be viable. I don't know if this is in the pipeline for libav. For my purposes I don't need RTP hinting anymore, but a fast start option would be very welcome. > If someone has a concrete usecase where some other parameter needs to be > adjusted, where the default isn't ok, we can of course try to make that > configurable, as we're discussing with the LATM case right now. Sure, this sounds nice, but most people who look at the tools will see that many already recommend a combination of ffmpeg and mp4box and likely decide to use that. It's a very limited audience that will know you are listening and have time to wait for the features to be implemented. Anyway, I wasn't trying to start a discussion on the RTP hinting features of libav, just trying to point out that there are alternatives for special use-cases you might miss. /Gil
On Sun, 22 May 2011, Gil Pedersen wrote: > I'm just stating how it looks at the moment, and I agree that several of > the mp4box options could be implemented quite easily in libav. Other > options, like interleaving and fast start, will require more substantial > changes to the movenc muxer since they can't be realized using the > current implementation. Ie. you need to create a temporary file and > write everything to disk twice for it to be viable. I don't know if this > is in the pipeline for libav. For my purposes I don't need RTP hinting > anymore, but a fast start option would be very welcome. Yes, faststart still has to be done externally (although that doesn't have anything to do with RTP hinting). > > If someone has a concrete usecase where some other parameter needs to be > > adjusted, where the default isn't ok, we can of course try to make that > > configurable, as we're discussing with the LATM case right now. > > Sure, this sounds nice, but most people who look at the tools will see > that many already recommend a combination of ffmpeg and mp4box and > likely decide to use that. It's a very limited audience that will know > you are listening and have time to wait for the features to be > implemented. Yeah, and it's understandable that few know about it since it's quite new, while mp4box has been around for a long time. As for few knowing that we're willing to fix things if they ask for it - I guess it's how most of the development happens here - we implement things once there is a concrete need/usecase. The code in its current form works just fine for my use cases at least, allowing me to simplify my dependency chain significantly, getting rid of mp4box. // Martin
diff --git a/libavformat/avformat.h b/libavformat/avformat.h index aca246d..f05bab3 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -719,7 +719,7 @@ typedef struct AVFormatContext { #define AVFMT_FLAG_IGNDTS 0x0008 ///< Ignore DTS on frames that contain both DTS & PTS #define AVFMT_FLAG_NOFILLIN 0x0010 ///< Do not infer any values from other values, just return what is stored in the container #define AVFMT_FLAG_NOPARSE 0x0020 ///< Do not use AVParsers, you also must set AVFMT_FLAG_NOFILLIN as the fillin code works on frames and no parsing -> no frames. Also seeking to frames can not work if parsing to find frame boundaries has been disabled -#define AVFMT_FLAG_RTP_HINT 0x0040 ///< Add RTP hinting to the output file +#define AVFMT_FLAG_RTP_HINT 0x0040 ///< Deprecated, use the rtphinting muxer specific AVOption instead int loop_input; diff --git a/libavformat/movenc.c b/libavformat/movenc.c index 0b78744..a1de7a6 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -38,6 +38,7 @@ #include <assert.h> static const AVOption options[] = { + { "rtphinting", "Add RTP hinting", offsetof(MOVMuxContext, rtp_hinting), FF_OPT_TYPE_INT, {.dbl = 0}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM }, { "latm", "Use MP4A-LATM RTP packetization instead of MPEG4-GENERIC for AAC", offsetof(MOVMuxContext, rtp_mp4a_latm), FF_OPT_TYPE_INT, {.dbl = 0}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM }, { NULL }, }; @@ -2144,6 +2145,11 @@ static int mov_write_header(AVFormatContext *s) mov->chapter_track = mov->nb_streams++; if (s->flags & AVFMT_FLAG_RTP_HINT) { + av_log(s, AV_LOG_WARNING, "The RTP_HINT flag is deprecated, enable it " + "via the -rtphinting 1 muxer option instead.\n"); + mov->rtp_hinting = 1; + } + if (mov->rtp_hinting) { /* Add hint tracks for each audio and video stream */ hint_track = mov->nb_streams; for (i = 0; i < s->nb_streams; i++) { @@ -2239,7 +2245,7 @@ static int mov_write_header(AVFormatContext *s) if (mov->chapter_track) mov_create_chapter_track(s, mov->chapter_track); - if (s->flags & AVFMT_FLAG_RTP_HINT) { + if (mov->rtp_hinting) { /* Initialize the hint tracks for each audio and video stream */ for (i = 0; i < s->nb_streams; i++) { AVStream *st = s->streams[i]; diff --git a/libavformat/movenc.h b/libavformat/movenc.h index a861324..68fe98c 100644 --- a/libavformat/movenc.h +++ b/libavformat/movenc.h @@ -110,6 +110,7 @@ typedef struct MOVMuxContext { uint64_t mdat_size; MOVTrack *tracks; + int rtp_hinting; int rtp_mp4a_latm; } MOVMuxContext; diff --git a/libavformat/options.c b/libavformat/options.c index 22807c3..4c3b8a5 100644 --- a/libavformat/options.c +++ b/libavformat/options.c @@ -49,7 +49,7 @@ static const AVOption options[]={ {"nofillin", "do not fill in missing values that can be exactly calculated", 0, FF_OPT_TYPE_CONST, {.dbl = AVFMT_FLAG_NOFILLIN }, INT_MIN, INT_MAX, D, "fflags"}, {"noparse", "disable AVParsers, this needs nofillin too", 0, FF_OPT_TYPE_CONST, {.dbl = AVFMT_FLAG_NOPARSE }, INT_MIN, INT_MAX, D, "fflags"}, {"igndts", "ignore dts", 0, FF_OPT_TYPE_CONST, {.dbl = AVFMT_FLAG_IGNDTS }, INT_MIN, INT_MAX, D, "fflags"}, -{"rtphint", "add rtp hinting", 0, FF_OPT_TYPE_CONST, {.dbl = AVFMT_FLAG_RTP_HINT }, INT_MIN, INT_MAX, E, "fflags"}, +{"rtphint", "add rtp hinting (deprecated, use the -rtphinting 1 option to the mov/3gp/mp4 muxer)", 0, FF_OPT_TYPE_CONST, {.dbl = AVFMT_FLAG_RTP_HINT }, INT_MIN, INT_MAX, E, "fflags"}, {"analyzeduration", "how many microseconds are analyzed to estimate duration", OFFSET(max_analyze_duration), FF_OPT_TYPE_INT, {.dbl = 5*AV_TIME_BASE }, 0, INT_MAX, D}, {"cryptokey", "decryption key", OFFSET(key), FF_OPT_TYPE_BINARY, {.dbl = 0}, 0, 0, D}, {"indexmem", "max memory used for timestamp index (per stream)", OFFSET(max_index_size), FF_OPT_TYPE_INT, {.dbl = 1<<20 }, 0, INT_MAX, D},