[8/8] movenc: Deprecate the global RTP hinting flag, use a private AVOption instead

Message ID 1305893640-56569-9-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö May 20, 2011, 12:14 p.m.
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(-)

Comments

Ronald Bultje May 20, 2011, 10:24 p.m. | #1
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
Gil Pedersen May 21, 2011, 10:31 a.m. | #2
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
Martin Storsjö May 21, 2011, 10:58 a.m. | #3
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
Gil Pedersen May 22, 2011, 10:11 a.m. | #4
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
Martin Storsjö May 22, 2011, 10:17 a.m. | #5
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
Gil Pedersen May 22, 2011, 10:40 a.m. | #6
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
Martin Storsjö May 22, 2011, 10:57 a.m. | #7
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
Gil Pedersen May 22, 2011, 5:16 p.m. | #8
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
Martin Storsjö May 22, 2011, 5:56 p.m. | #9
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

Patch

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},