Re: [Libquicktime-devel] More patches

[Burkhard P. <pl...@ip...>]

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