Menu
▾
▴
libquicktime-devel
|
From: Joseph A. <jo...@mi...> - 2012-11-19 11:59:54
|
Hi, I've got 3 patches this time. 1. uninitialized_read.patch Fixes reading uninitialized memory or even (if you are unlucky) invalid memory. 2. sdtp_for_xdcam.patch Fixes a few things that were wrong with my XDCAM encoding code and introducing support for sdtp atom. These changes make it possible to load XDCAM files generated by libquicktime into Avid Media Composer. One interesting thing is Avid 6.5 fails to load XDCAM files with timing of 1/25, while 24/600 (which is the same) works fine. Avid 5.5 doesn't have this problem. As part of those changes I took libery to unconditionally enable (it was #ifdef DO_INTERLACING before) setting AVFrame->interlaced_frame and AVFrame->top_field_first according to libquicktime's interlacing setting. I didn't enable setting interlaced DCT though, as I prefer individual codecs to set this up. That's not a big deal anyway. Now, you might think top_field_first is also not a big deal, but that's only if you use Windows QuickTime or libquicktime API to decode frames, as neither of them support decoding individual fields. Now, I know for sure that Avid Media Composer uses Windows QuickTime API to read compressed frames but uses it's own decoder to decode them. Therefore, field order is important. 3. Fixes a problem with presentation time stamps that get fed into ffmpeg. Initially I thought that was a bug in FFMpeg and even sent them a patch. In the discussion that followed it became clear it's libquicktime's bug, not FFMpeg's one. Here is the discussion on the ffmpeg mailing list: http://comments.gmane.org/gmane.comp.video.ffmpeg.devel/154243 I am not 100% sure this patch won't cause side-effects, such as inability to encode variable-length frames, therefore I made it a separate patch, rather than including it in the XDCAM patch. -- Joseph Artsimovich Senior C++ Applications Developer MirriAd Ltd |
|
From: Burkhard P. <pl...@ip...> - 2012-11-19 17:25:22
|
Hi, Am 19.11.2012 12:59, schrieb Joseph Artsimovich: > Hi, > > I've got 3 patches this time. > > 1. uninitialized_read.patch > Fixes reading uninitialized memory or even (if you are unlucky) invalid memory. > Applied. > 2. sdtp_for_xdcam.patch > Fixes a few things that were wrong with my XDCAM encoding code and introducing support for sdtp atom. These changes make it possible to load XDCAM files generated by libquicktime into Avid Media Composer. One interesting thing is Avid 6.5 fails to load XDCAM files with timing of 1/25, while 24/600 > (which is the same) works fine. Avid 5.5 doesn't have this problem. Yea for some reasons, the timescale of 600 occurs quite often on quicktime files. Making a software depend on this is not a good idea though. > As part of those changes I took libery to unconditionally enable (it was #ifdef DO_INTERLACING before) setting AVFrame->interlaced_frame and AVFrame->top_field_first according to libquicktime's interlacing setting. I didn't enable setting interlaced DCT though, as I prefer individual codecs to set > this up. That's not a big deal anyway. Now, you might think top_field_first is also not a big deal, but that's only if you use Windows QuickTime or libquicktime API to decode frames, as neither of them support decoding individual fields. Now, I know for sure that Avid Media Composer uses Windows > QuickTime API to read compressed frames but uses it's own decoder to decode them. Therefore, field order is important. That DO_INTERLACING macro comes from an old attempt to support interlaced MPEG-4 encoding (which failed miserably). Of course interlaced dct is the important flag. I hope without it the top_field_first and interlaced_frame fields are ignored. Applied this one also. > 3. Fixes a problem with presentation time stamps that get fed into ffmpeg. Initially I thought that was a bug in FFMpeg and > even sent them a patch. In the discussion that followed it became clear it's libquicktime's bug, not FFMpeg's one. Here is > the discussion on the ffmpeg mailing list: > http://comments.gmane.org/gmane.comp.video.ffmpeg.devel/154243 > I am not 100% sure this patch won't cause side-effects, such as inability to encode variable-length frames, therefore I made > it a separate patch, rather than including it in the XDCAM patch. I'm 100% sure it *will* have the side effect of completely disabling VFR support. I know that most people have CFR streams, but Quicktime is one of the few formats with proper VFR support and a patch, which removes this support from a codec is not acceptable. On the other hand I understand the issue and that proper PTS values are important for rate control. From what I see right now, the lines: codec->avctx->time_base.den = lqt_video_time_scale(file, track); codec->avctx->time_base.num = lqt_frame_duration(file, track, NULL); should in any case become: codec->avctx->time_base.den = lqt_video_time_scale(file, track); codec->avctx->time_base.num = 1; and (citing Michael Niedermayer): "if one would set timebase = 1/600 THEN pts would go 0 24 48 ..." which would also be libquicktime's idea of PTSes in a 600/24 fps stream. Either I completely overlooked something, or the 1 line change above is enough. Burkhard |
|
From: Christian E. <bla...@gm...> - 2012-11-20 00:08:22
|
* Burkhard Plaum on Monday, November 19, 2012 at 18:24:56 +0100
> Am 19.11.2012 12:59, schrieb Joseph Artsimovich:
>> I've got 3 patches this time.
>>
>> 1. uninitialized_read.patch
>> Fixes reading uninitialized memory or even (if you are unlucky) invalid memory.
>
> Applied.
These latest checkins break compilation on MacOS 10.5.8:
[...]
/bin/sh ../libtool --tag=CC --mode=compile gcc -DHAVE_CONFIG_H -I. -I.. -I../include -I../include -O3 -I/usr/local/include -I/sw/include -DLOCALE_DIR=\"/usr/local/share/locale\" -O3 -I/usr/local/include -I/sw/include -finline-functions -Wall -Winline -Wmissing-declarations -Wdeclaration-after-statement -fvisibility=hidden -MT stps.lo -MD -MP -MF .deps/stps.Tpo -c -o stps.lo stps.c
libtool: compile: gcc -DHAVE_CONFIG_H -I. -I.. -I../include -I../include -O3 -I/usr/local/include -I/sw/include -DLOCALE_DIR=\"/usr/local/share/locale\" -O3 -I/usr/local/include -I/sw/include -finline-functions -Wall -Winline -Wmissing-declarations -Wdeclaration-after-statement -fvisibility=hidden -MT stps.lo -MD -MP -MF .deps/stps.Tpo -c stps.c -fno-common -DPIC -o .libs/stps.o
mv -f .deps/stps.Tpo .deps/stps.Plo
make[2]: *** No rule to make target `sdtp.c', needed by `sdtp.lo'. Stop.
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2
--
\black\trash movie _COWBOY CANOE COMA_
Ein deutscher Western/A German Western
--->> http://www.blacktrash.org/underdogma/ccc.php
|
|
From: Joseph A. <jo...@mi...> - 2012-11-20 08:56:20
|
On 20/11/2012 00:08, Christian Ebert wrote: > ... > These latest checkins break compilation on MacOS 10.5.8: > > ... > make[2]: *** No rule to make target `sdtp.c', needed by `sdtp.lo'. Stop. I believe you need to re-run autogen.sh -- Joseph Artsimovich Senior C++ Applications Developer MirriAd Ltd |
|
From: Joseph A. <jo...@mi...> - 2012-11-20 08:58:57
|
On 19/11/2012 17:24, Burkhard Plaum wrote: > ... > > From what I see right now, the lines: > > codec->avctx->time_base.den = lqt_video_time_scale(file, track); > codec->avctx->time_base.num = lqt_frame_duration(file, track, NULL); > > should in any case become: > > codec->avctx->time_base.den = lqt_video_time_scale(file, track); > codec->avctx->time_base.num = 1; > > and (citing Michael Niedermayer): > > "if one would set timebase = 1/600 THEN pts would go 0 24 48 ..." > > which would also be libquicktime's idea of PTSes in a 600/24 fps stream. > Either I completely overlooked something, or the 1 line change above is enough. OK, I'll try to implement that strategy. -- Joseph Artsimovich Senior C++ Applications Developer MirriAd Ltd |
|
From: Burkhard P. <pl...@ip...> - 2012-11-20 09:52:53
|
Hi, Am 20.11.2012 09:55, schrieb Joseph Artsimovich: > On 20/11/2012 00:08, Christian Ebert wrote: >> ... >> These latest checkins break compilation on MacOS 10.5.8: >> >> ... >> make[2]: *** No rule to make target `sdtp.c', needed by `sdtp.lo'. Stop. > I believe you need to re-run autogen.sh > No, I forgot to add sdtp.c to the repository. Fixed. Burkhard |
|
From: Christian E. <bla...@gm...> - 2012-11-20 14:07:16
|
* Burkhard Plaum on Tuesday, November 20, 2012 at 10:52:28 +0100 > Am 20.11.2012 09:55, schrieb Joseph Artsimovich: >> On 20/11/2012 00:08, Christian Ebert wrote: >>> These latest checkins break compilation on MacOS 10.5.8: >>> >>> make[2]: *** No rule to make target `sdtp.c', needed by `sdtp.lo'. Stop. >> I believe you need to re-run autogen.sh As I always make distclean, I always have to re-run autogen.sh. > No, I forgot to add sdtp.c to the repository. Fixed. yup, confirmed, thanks a lot! c -- \black\trash movie _SAME TIME SAME PLACE_ --->> http://www.blacktrash.org/underdogma/stsp.php \black\trash audio _ANOTHER TIME ANOTHER PLACE_ --->> http://www.blacktrash.org/underdogma/atap.html |
|
From: Joseph A. <jo...@mi...> - 2012-11-20 17:12:12
|
On 20/11/2012 08:58, Joseph Artsimovich wrote: > On 19/11/2012 17:24, Burkhard Plaum wrote: > >> ... >> >> From what I see right now, the lines: >> >> codec->avctx->time_base.den = lqt_video_time_scale(file, track); >> codec->avctx->time_base.num = lqt_frame_duration(file, track, NULL); >> >> should in any case become: >> >> codec->avctx->time_base.den = lqt_video_time_scale(file, track); >> codec->avctx->time_base.num = 1; >> >> and (citing Michael Niedermayer): >> >> "if one would set timebase = 1/600 THEN pts would go 0 24 48 ..." >> >> which would also be libquicktime's idea of PTSes in a 600/24 fps stream. >> Either I completely overlooked something, or the 1 line change above is enough. > OK, I'll try to implement that strategy. > It doesn't look like it's going to work. When I set the following: codec->avctx->time_base.num = 1; codec->avctx->time_base.den = lqt_video_time_scale(file, track); codec->avctx->ticks_per_frame = lqt_frame_duration(file, track, NULL); avcodec_open2() then fails with "MPEG1/2 does not support 600/1 fps". It doesn't even look at ticks_per_frame. It also lools like even if you remove that check, 1/600 ratio would get written to MPEG bitstream. In fact that's the whole point of ticks_per_frame, to be able to put 1/50 to MPEG bitstream for 50i content. So, this behaviour is by design. The solution I am thinking about is doing what my previous patch did, but only for XDCAM. This will make it impossible to generate variable-duration XDCAM frames, but I don't expect anyone to ever need that. -- Joseph Artsimovich Senior C++ Applications Developer MirriAd Ltd |
|
From: Burkhard P. <pl...@ip...> - 2012-11-21 10:19:18
|
Hi,
Am 20.11.2012 18:11, schrieb Joseph Artsimovich:
> It doesn't look like it's going to work. When I set the following:
>
> codec->avctx->time_base.num = 1;
> codec->avctx->time_base.den = lqt_video_time_scale(file, track);
> codec->avctx->ticks_per_frame = lqt_frame_duration(file, track, NULL);
>
> avcodec_open2() then fails with "MPEG1/2 does not support 600/1 fps". It
> doesn't even look at ticks_per_frame.
Ahh ok, now I remember it wasn't that easy. And of course framerate isn't
only used for rate control, it's also be written to the MPEG sequence header :)
ticks_per_frame is a field in some MPEG4 header so I'd guess it's used
mainly by CODEC_ID_MPEG4.
> It also lools like even if you remove that check, 1/600 ratio would get
> written to MPEG bitstream.
No it won't, because MPEG-1/2 has no way to way to store arbitrary
framerates.
> In fact that's the whole point of
> ticks_per_frame, to be able to put 1/50 to MPEG bitstream for 50i
> content. So, this behaviour is by design.
>
> The solution I am thinking about is doing what my previous patch did,
> but only for XDCAM. This will make it impossible to generate
> variable-duration XDCAM frames, but I don't expect anyone to ever need that.
I would make it depend on the codec_id because it also applied to IMX.
For CODEC_ID_MPEG2VIDEO (and probably others) set time_base to the framerate
like you did earlier, and increase PTS by one for each frame.
Just found this snippet in my other libavcodec frontend:
static void set_framerate(ffmpeg_video_stream_t * st)
{
if(st->format.framerate_mode == GAVL_FRAMERATE_CONSTANT)
{
st->stream->codec->time_base.den = st->format.timescale;
st->stream->codec->time_base.num = st->format.frame_duration;
}
else
{
st->stream->codec->time_base.den = st->format.timescale;
st->stream->codec->time_base.num = 1;
}
}
Burkhard
|
|
From: Joseph A. <jo...@mi...> - 2012-11-21 12:50:21
|
On 21/11/2012 10:18, Burkhard Plaum wrote:
> ...
>
>
> I would make it depend on the codec_id because it also applied to IMX.
> For CODEC_ID_MPEG2VIDEO (and probably others) set time_base to the framerate
> like you did earlier, and increase PTS by one for each frame.
>
> Just found this snippet in my other libavcodec frontend:
>
> static void set_framerate(ffmpeg_video_stream_t * st)
> {
> if(st->format.framerate_mode == GAVL_FRAMERATE_CONSTANT)
> {
> st->stream->codec->time_base.den = st->format.timescale;
> st->stream->codec->time_base.num = st->format.frame_duration;
> }
> else
> {
> st->stream->codec->time_base.den = st->format.timescale;
> st->stream->codec->time_base.num = 1;
> }
> }
Are you proposing making time_base.num = 1 for everything but
CODEC_ID_MPEG2VIDEO? I think that would cause problems with other
codecs. For instance, ffmpeg's dnxhd encoder chooses encoding profile by
matching bitrate, frame rate and frame size.
What I was proposing is introducing a pts divisor/multiplier that will
be 1 except for XDCAM, where it will be equal to time scale.
--
Joseph Artsimovich
Senior C++ Applications Developer
MirriAd Ltd
|
|
From: Burkhard P. <pl...@ip...> - 2012-11-22 10:19:15
|
Hi,
Am 21.11.2012 13:50, schrieb Joseph Artsimovich:
> On 21/11/2012 10:18, Burkhard Plaum wrote:
>> ...
>>
>>
>> I would make it depend on the codec_id because it also applied to IMX.
>> For CODEC_ID_MPEG2VIDEO (and probably others) set time_base to the framerate
>> like you did earlier, and increase PTS by one for each frame.
>>
>> Just found this snippet in my other libavcodec frontend:
>>
>> static void set_framerate(ffmpeg_video_stream_t * st)
>> {
>> if(st->format.framerate_mode == GAVL_FRAMERATE_CONSTANT)
>> {
>> st->stream->codec->time_base.den = st->format.timescale;
>> st->stream->codec->time_base.num = st->format.frame_duration;
>> }
>> else
>> {
>> st->stream->codec->time_base.den = st->format.timescale;
>> st->stream->codec->time_base.num = 1;
>> }
>> }
> Are you proposing making time_base.num = 1 for everything but
> CODEC_ID_MPEG2VIDEO?
For everything which supports variable framerates.
> I think that would cause problems with other
> codecs. For instance, ffmpeg's dnxhd encoder chooses encoding profile by
> matching bitrate, frame rate and frame size.
I think for DV it's the same. This would mean that at least dnxhd, DV and MPEG-2
are constant frame rate while most others might be more flexible.
Burkhard
|
|
From: Joseph A. <jo...@mi...> - 2012-11-23 10:00:39
Attachments:
pts_v2.patch
|
On 22/11/2012 10:18, Burkhard Plaum wrote: > I think for DV it's the same. This would mean that at least dnxhd, DV and MPEG-2 > are constant frame rate while most others might be more flexible. Take a look at the attached patch. Was that the aproach you had in mind? -- Joseph Artsimovich Senior C++ Applications Developer MirriAd Ltd |
|
From: Burkhard P. <pl...@ip...> - 2012-11-23 16:11:53
|
Hi, Am 23.11.2012 11:00, schrieb Joseph Artsimovich: > On 22/11/2012 10:18, Burkhard Plaum wrote: >> I think for DV it's the same. This would mean that at least dnxhd, DV and MPEG-2 >> are constant frame rate while most others might be more flexible. > Take a look at the attached patch. Was that the aproach you had in mind? > Yes, that looks good. Did you try it? Burkhard |
|
From: Joseph A. <jo...@mi...> - 2012-11-23 16:15:14
|
On 23/11/2012 16:11, Burkhard Plaum wrote: > Hi, > > Am 23.11.2012 11:00, schrieb Joseph Artsimovich: >> On 22/11/2012 10:18, Burkhard Plaum wrote: >>> I think for DV it's the same. This would mean that at least dnxhd, DV and MPEG-2 >>> are constant frame rate while most others might be more flexible. >> Take a look at the attached patch. Was that the aproach you had in mind? >> > Yes, that looks good. Did you try it? I worked fine on a few tests I did. -- Joseph Artsimovich Senior C++ Applications Developer MirriAd Ltd |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X