Message ID | 20161011172444.4295-2-al4321@gmail.com |
---|---|
State | New |
Headers | show |
On 11/10/2016 18:24, Alexey Eromenko wrote: > --- > libavformat/movenc.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c > index 8b4aa5f..0e2fc55 100644 > --- a/libavformat/movenc.c > +++ b/libavformat/movenc.c > @@ -5666,16 +5666,20 @@ static int mov_write_header(AVFormatContext *s) > while(track->timescale < 10000) > track->timescale *= 2; > } > + if (track->timescale > 100000 && (!mov->video_track_timescale)) { As I said before, having this as 'default' behaviour would interfere with large, but correct timescales. This should only be a suggestion to the user. > + unsigned int timescale_new = (unsigned int)((double)(st->time_base.den) > + * 1000 / (double)(st->time_base.num)); You surely don't need all these casts. > + av_log(s, AV_LOG_WARNING, > + "WARNING codec timebase is very high. If duration is too long,\n" > + "file may not be playable by Apple Quicktime. Auto-setting\n" > + "a shorter timebase %u instead of %d.\n", timescale_new, track->timescale); > + track->timescale = timescale_new; > + } > if (st->codecpar->width > 65535 || st->codecpar->height > 65535) { > av_log(s, AV_LOG_ERROR, "Resolution %dx%d too large for mov/mp4\n", st->codecpar->width, st->codecpar->height); > ret = AVERROR(EINVAL); > goto error; > } > - if (track->mode == MODE_MOV && track->timescale > 100000) > - av_log(s, AV_LOG_WARNING, > - "WARNING codec timebase is very high. If duration is too long,\n" > - "file may not be playable by quicktime. Specify a shorter timebase\n" > - "or choose different container.\n"); Keep the logic in the same place please. > if (track->mode == MODE_MOV && > track->par->codec_id == AV_CODEC_ID_RAWVIDEO && > track->tag == MKTAG('r','a','w',' ')) { >
On Tue, Oct 11, 2016 at 8:29 PM, Josh de Kock <josh@itanimul.li> wrote: > On 11/10/2016 18:24, Alexey Eromenko wrote: >> >> --- >> libavformat/movenc.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/libavformat/movenc.c b/libavformat/movenc.c >> index 8b4aa5f..0e2fc55 100644 >> --- a/libavformat/movenc.c >> +++ b/libavformat/movenc.c >> @@ -5666,16 +5666,20 @@ static int mov_write_header(AVFormatContext *s) >> while(track->timescale < 10000) >> track->timescale *= 2; >> } >> + if (track->timescale > 100000 && >> (!mov->video_track_timescale)) { > > As I said before, having this as 'default' behaviour would interfere with > large, but correct timescales. This should only be a suggestion to the user. > This happens only for very high timescale from source video, and it seems to work on Apple QuickTime/iTunes, WMP and VLC. For the vast majority videos (99%+), I _do not_ touch the timebase. And when I do touch, I give a WARNING to the user. Plus I offer to override this decision via a parameter. It should be stable and regression-free for most of our users. Is there any downside for this approach ? >> + unsigned int timescale_new = (unsigned >> int)((double)(st->time_base.den) >> + * 1000 / (double)(st->time_base.num)); > > You surely don't need all these casts. Unless I'm guaranteed to get int64 on ALL platforms, I don't see how-to write this code without casts to double-float. Int32 will over-flow, even unsigned. I can do the multiply after the division, but afraid of losing precision. And since this is not a real-time code anyway, I consider this an "okay" trade-off. Feel free to replace it with a better alternative. >> + av_log(s, AV_LOG_WARNING, >> + "WARNING codec timebase is very high. If duration >> is too long,\n" >> + "file may not be playable by Apple Quicktime. >> Auto-setting\n" >> + "a shorter timebase %u instead of %d.\n", >> timescale_new, track->timescale); >> + track->timescale = timescale_new; >> + } >> if (st->codecpar->width > 65535 || st->codecpar->height > >> 65535) { >> av_log(s, AV_LOG_ERROR, "Resolution %dx%d too large for >> mov/mp4\n", st->codecpar->width, st->codecpar->height); >> ret = AVERROR(EINVAL); >> goto error; >> } >> - if (track->mode == MODE_MOV && track->timescale > 100000) >> - av_log(s, AV_LOG_WARNING, >> - "WARNING codec timebase is very high. If duration >> is too long,\n" >> - "file may not be playable by quicktime. Specify a >> shorter timebase\n" >> - "or choose different container.\n"); > > Keep the logic in the same place please. Logically timescale part 1 and timescale part 2 belong together, so I grouped them together. if (mov->video_track_timescale) { track->timescale = mov->video_track_timescale; } else { and if (track->timescale > 100000) { unsigned int timescale_new = (unsigned int)((double)(st->time_base.den) Feel free to modify my code and commit, after we reach some rough consensus on the matters.
>>> + unsigned int timescale_new = (unsigned >>> int)((double)(st->time_base.den) >>> + * 1000 / (double)(st->time_base.num)); >> >> You surely don't need all these casts. > > Unless I'm guaranteed to get int64 on ALL platforms, I don't see > how-to write this code without casts to double-float. Int32 will > over-flow, even unsigned. > I can do the multiply after the division, but afraid of losing precision. > And since this is not a real-time code anyway, I consider this an > "okay" trade-off. > Feel free to replace it with a better alternative. > Do you suggest moving the multiplication after the division ? -Alexey
Hi! > Am 11.10.2016 um 19:24 schrieb Alexey Eromenko <al4321@gmail.com>: > - if (track->mode == MODE_MOV This change is not acceptable: The problem only affects mov, not isom. Mentioning the option name in the existing warning message is of course a good idea. Carl Eugen
On Tuesday, 11 October 2016, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > Hi! > > > Am 11.10.2016 um 19:24 schrieb Alexey Eromenko <al4321@gmail.com > <javascript:;>>: > > > > - if (track->mode == MODE_MOV > > This change is not acceptable: > The problem only affects mov, not isom. > Mentioning the option name in the existing warning message is of course a > good idea. The problem affects both MOV and MP4 on Apple decoders. My goal is to create a video file that is cross platform, works everywhere. The only way to achieve that goal is to use a subset of MP4 specification. As long as my patch doesn't break video for other people, and fixes this use case, I recommend to apply it.
Hi, On Oct 11, 2016 2:40 PM, "Alexey Eromenko" <al4321@gmail.com> wrote: > > On Tue, Oct 11, 2016 at 8:29 PM, Josh de Kock <josh@itanimul.li> wrote: > > On 11/10/2016 18:24, Alexey Eromenko wrote: > >> > >> --- > >> libavformat/movenc.c | 14 +++++++++----- > >> 1 file changed, 9 insertions(+), 5 deletions(-) > >> > >> diff --git a/libavformat/movenc.c b/libavformat/movenc.c > >> index 8b4aa5f..0e2fc55 100644 > >> --- a/libavformat/movenc.c > >> +++ b/libavformat/movenc.c > >> @@ -5666,16 +5666,20 @@ static int mov_write_header(AVFormatContext *s) > >> while(track->timescale < 10000) > >> track->timescale *= 2; > >> } > >> + if (track->timescale > 100000 && > >> (!mov->video_track_timescale)) { > > > > As I said before, having this as 'default' behaviour would interfere with > > large, but correct timescales. This should only be a suggestion to the user. > > > > This happens only for very high timescale from source video, and it > seems to work on Apple QuickTime/iTunes, WMP and VLC. > For the vast majority videos (99%+), I _do not_ touch the timebase. > And when I do touch, I give a WARNING to the user. > Plus I offer to override this decision via a parameter. > It should be stable and regression-free for most of our users. > Is there any downside for this approach ? > > >> + unsigned int timescale_new = (unsigned > >> int)((double)(st->time_base.den) > >> + * 1000 / (double)(st->time_base.num)); > > > > You surely don't need all these casts. > > Unless I'm guaranteed to get int64 on ALL platforms, I don't see > how-to write this code without casts to double-float. Int32 will > over-flow, even unsigned. > I can do the multiply after the division, but afraid of losing precision. > And since this is not a real-time code anyway, I consider this an > "okay" trade-off. > Feel free to replace it with a better alternative. We use int64_t all over the place, it's OK to rely on it since it guarantees the same result across archs, which floats don't. Ronald
diff --git a/libavformat/movenc.c b/libavformat/movenc.c index 8b4aa5f..0e2fc55 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -5666,16 +5666,20 @@ static int mov_write_header(AVFormatContext *s) while(track->timescale < 10000) track->timescale *= 2; } + if (track->timescale > 100000 && (!mov->video_track_timescale)) { + unsigned int timescale_new = (unsigned int)((double)(st->time_base.den) + * 1000 / (double)(st->time_base.num)); + av_log(s, AV_LOG_WARNING, + "WARNING codec timebase is very high. If duration is too long,\n" + "file may not be playable by Apple Quicktime. Auto-setting\n" + "a shorter timebase %u instead of %d.\n", timescale_new, track->timescale); + track->timescale = timescale_new; + } if (st->codecpar->width > 65535 || st->codecpar->height > 65535) { av_log(s, AV_LOG_ERROR, "Resolution %dx%d too large for mov/mp4\n", st->codecpar->width, st->codecpar->height); ret = AVERROR(EINVAL); goto error; } - if (track->mode == MODE_MOV && track->timescale > 100000) - av_log(s, AV_LOG_WARNING, - "WARNING codec timebase is very high. If duration is too long,\n" - "file may not be playable by quicktime. Specify a shorter timebase\n" - "or choose different container.\n"); if (track->mode == MODE_MOV && track->par->codec_id == AV_CODEC_ID_RAWVIDEO && track->tag == MKTAG('r','a','w',' ')) {