libquicktime-devel — Discussion about development

Re: [Libquicktime-devel] [PATCH] Compatibility with Omneon hardware

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

Hi,

Am 18.12.2012 16:01, schrieb Joseph Artsimovich:
> Hi,
>
> We've got a file from a client that didn't load with libquicktime. The file was generated by Omneon hardware.
>
> It turns out they put some zero bytes into the "edts" atom following its "elst" children. The number of zero
 > bytes not being a multiple of 8 causes stream desynchronization in libquicktime. The attached patch fixes this
 > particular problem, although I suspect there are many places in libquicktime
> where such desynchronization is possible.

Applied

What annoys even more is, that these zero bytes are not documented anywhere,
or I just didn't find it. There is a similar hack in stsdtable.c already.

Burkhard

[Libquicktime-devel] [PATCH] Compatibility with Omneon hardware

[Joseph A. <jo...@mi...>]

Hi,

We've got a file from a client that didn't load with libquicktime. The 
file was generated by Omneon hardware.

It turns out they put some zero bytes into the "edts" atom following its 
"elst" children. The number of zero bytes not being a multiple of 8 
causes stream desynchronization in libquicktime. The attached patch 
fixes this particular problem, although I suspect there are many places 
in libquicktime where such desynchronization is possible.

-- 
Joseph Artsimovich
Senior C++ Applications Developer
MirriAd Ltd

Re: [Libquicktime-devel] [PATCH] Set vendor to "appl"

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

Hi,

Am 27.11.2012 10:32, schrieb Joseph Artsimovich:
[...]
>
> Therefore, a patch is attached changing vendor from "lqt " to "appl".

Applied, but with a comment that it's a workaround for broken decoders.

In an ideal world, the video format details would be clearly defined
by the relevant entries in the sample description table and the vendor
field would be purely informational. Therefore I changed in to "lqt "
a while ago, because libquicktime is not Apple. But apparently telling
the truth is not always the best option.

Anyway, good that you figured that out.

> P.S: Also please apply my previous patch.

Done

Burkhard

[Libquicktime-devel] [PATCH] Set vendor to "appl"

[Joseph A. <jo...@mi...>]

Yet another Avid Media Composer compatibility patch.

I've noticed that some of the files generated by libquicktime produce a 
colorshift when loaded into Avid in AMA (as opposed to Import) mode. 
Further investigation revealed exactly what was going on:

Avid calls FindNextComponent()  from Windows QuickTime API in order to 
query its supported output formats. FindNextComponent() allows querying 
by component type (image decoder, sound encoder, ...), fourcc, vendor, 
or any combination of those. As you probably guessed by now, Avid 
includes vendor in the query. As a result, it's unable to query the 
decoder for its supported output formats and defaults to 8-bit ARGB. On 
files produced by Apple QuickTime libraries it finds some supported 
YUV-based formats and asks QuickTime to output that, converting to RGB 
later on its own. Now guess what, with some codecs the result of 
QuickTime doing YUV to RGB conversion differs from Avid doing the same!
I don't really care which one of them is right, I only care that files 
we produce with libquicktime are visually indistinguishable in any 
software from files Apple QuickTime libraries would produce.

Therefore, a patch is attached changing vendor from "lqt " to "appl".

P.S: Also please apply my previous patch.

-- 
Joseph Artsimovich
Senior C++ Applications Developer
MirriAd Ltd

Re: [Libquicktime-devel] More patches

[Joseph A. <jo...@mi...>]

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

Re: [Libquicktime-devel] More patches

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

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

Re: [Libquicktime-devel] More patches

[Joseph A. <jo...@mi...>]

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

Re: [Libquicktime-devel] More patches

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

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

Re: [Libquicktime-devel] More patches

[Joseph A. <jo...@mi...>]

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

Re: [Libquicktime-devel] More patches

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

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

Re: [Libquicktime-devel] More patches

[Joseph A. <jo...@mi...>]

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

Re: [Libquicktime-devel] More patches

[Christian E. <bla...@gm...>]

* 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

Re: [Libquicktime-devel] More patches

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

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

Re: [Libquicktime-devel] More patches

[Joseph A. <jo...@mi...>]

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

Re: [Libquicktime-devel] More patches

[Joseph A. <jo...@mi...>]

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

Re: [Libquicktime-devel] More patches

[Christian E. <bla...@gm...>]

* 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

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

[Libquicktime-devel] More patches

[Joseph A. <jo...@mi...>]

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

Re: [Libquicktime-devel] My last patch broke things

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

Hi,

Am 09.11.2012 10:24, schrieb Joseph Artsimovich:
> On 08/11/2012 13:02, Burkhard Plaum wrote:
>> ...
>>
>>
>> I'd suggest the following:
>>
>> - Add a "do_encode" flag to quicktime_video_map_t
>> - Set this flag in quicktime_init_video_map(), there is an "int encode" argument
>> - Use the flag in quicktime_init_video_codec_ffmpeg().
>>
>> Note that the vtrack argument for quicktime_init_video_codec_ffmpeg() can also be
>> NULL when we are just testing if the codec supports writing compressed packets.
> That was exhaustive answer. Following it was a breeze.
> The patch is attached.

Applied it last week already. Just forgot to send this mail :)

Burkhard

Re: [Libquicktime-devel] My last patch broke things

[Joseph A. <jo...@mi...>]

On 08/11/2012 13:02, Burkhard Plaum wrote:
> ...
>
>
> I'd suggest the following:
>
> - Add a "do_encode" flag to quicktime_video_map_t
> - Set this flag in quicktime_init_video_map(), there is an "int encode" argument
> - Use the flag in quicktime_init_video_codec_ffmpeg().
>
> Note that the vtrack argument for quicktime_init_video_codec_ffmpeg() can also be
> NULL when we are just testing if the codec supports writing compressed packets.
That was exhaustive answer. Following it was a breeze.
The patch is attached.

-- 
Joseph Artsimovich
Senior C++ Applications Developer
MirriAd Ltd

Re: [Libquicktime-devel] My last patch broke things

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

Hi,

Am 08.11.2012 11:47, schrieb Joseph Artsimovich:
> Hi,
>
> So, my last patch was replacing:
> avcodec_alloc_context3(NULL);
> with:
> avcodec_alloc_context3(encoder ? encoder : decoder);
>
> I assumed either encoder or decoder would be set but not both. Now it
> turns out both are usually set, and are different ones. What's worse,
> passing a wrong one can cause memory corruption. The test case is
> decoding XDCAM and passing the encoder instead of the decoder.
>
> Now the problem is I couldn't find a way to figure out whether we are
> encoding or decoding.
> Burkhard, can you take a look?

Right now I don't see how to detect, if we are encoding or decoding.

I'd suggest the following:

- Add a "do_encode" flag to quicktime_video_map_t
- Set this flag in quicktime_init_video_map(), there is an "int encode" argument
- Use the flag in quicktime_init_video_codec_ffmpeg().

Note that the vtrack argument for quicktime_init_video_codec_ffmpeg() can also be
NULL when we are just testing if the codec supports writing compressed packets.

Burkhard

Re: [Libquicktime-devel] Prores codec issues.

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

Hi,

Am 08.11.2012 10:30, schrieb Al Crate:
> The better solution was to fix the actual bug..

indeed

> In the colorspace_macros.h
>
> #define YUV_10_TO_RGB_24(y,u,v,r,g,b) \
>     i_tmp=(yj_16_to_rgb * (y-0x40) + vj_16_to_r * (v-0x200))>>18;        \
>     r = RECLIP_8(i_tmp);\
>     i_tmp=(yj_16_to_rgb * (y-0x40) + uj_16_to_g * (u-0x200)+ vj_16_to_g *
> (v-0x200))>>18; \
>     g = RECLIP_8(i_tmp);\
>     i_tmp=(yj_16_to_rgb * (y-0x40) + uj_16_to_b * (u-0x200))>>18; \
>     b = RECLIP_8(i_tmp);
>
> should actually be
>
> #define YUV_10_TO_RGB_24(y,u,v,r,g,b) \
>     i_tmp=(y_16_to_rgb * (y-0x40) + v_16_to_r * (v-0x200))>>18;        \
>     r = RECLIP_8(i_tmp);\
>     i_tmp=(y_16_to_rgb * (y-0x40) + u_16_to_g * (u-0x200)+ v_16_to_g *
> (v-0x200))>>18; \
>     g = RECLIP_8(i_tmp);\
>     i_tmp=(y_16_to_rgb * (y-0x40) + u_16_to_b * (u-0x200))>>18; \
>     b = RECLIP_8(i_tmp);
>
> It was using the full range functions by mistake, output is now correct.

Applied. Next time please send a real patch.

Burkhard

[Libquicktime-devel] My last patch broke things

[Joseph A. <jo...@mi...>]

Hi,

So, my last patch was replacing:
avcodec_alloc_context3(NULL);
with:
avcodec_alloc_context3(encoder ? encoder : decoder);

I assumed either encoder or decoder would be set but not both. Now it 
turns out both are usually set, and are different ones. What's worse, 
passing a wrong one can cause memory corruption. The test case is 
decoding XDCAM and passing the encoder instead of the decoder.

Now the problem is I couldn't find a way to figure out whether we are 
encoding or decoding.
Burkhard, can you take a look?

-- 
Joseph Artsimovich
Senior C++ Applications Developer
MirriAd Ltd

Re: [Libquicktime-devel] Prores codec issues.

[Joseph A. <jo...@mi...>]

Oops, that bug was introduced by me. No surprise it went unnoticed for a 
while, as I am not using libquicktime's color conversion code.


On 08/11/2012 09:30, Al Crate wrote:
> The better solution was to fix the actual bug..
>
> In the colorspace_macros.h
>
> #define YUV_10_TO_RGB_24(y,u,v,r,g,b) \
>     i_tmp=(yj_16_to_rgb * (y-0x40) + vj_16_to_r * (v-0x200))>>18;        \
>     r = RECLIP_8(i_tmp);\
>     i_tmp=(yj_16_to_rgb * (y-0x40) + uj_16_to_g * (u-0x200)+ vj_16_to_g *
> (v-0x200))>>18; \
>     g = RECLIP_8(i_tmp);\
>     i_tmp=(yj_16_to_rgb * (y-0x40) + uj_16_to_b * (u-0x200))>>18; \
>     b = RECLIP_8(i_tmp);
>
> should actually be
>
> #define YUV_10_TO_RGB_24(y,u,v,r,g,b) \
>     i_tmp=(y_16_to_rgb * (y-0x40) + v_16_to_r * (v-0x200))>>18;        \
>     r = RECLIP_8(i_tmp);\
>     i_tmp=(y_16_to_rgb * (y-0x40) + u_16_to_g * (u-0x200)+ v_16_to_g *
> (v-0x200))>>18; \
>     g = RECLIP_8(i_tmp);\
>     i_tmp=(y_16_to_rgb * (y-0x40) + u_16_to_b * (u-0x200))>>18; \
>     b = RECLIP_8(i_tmp);
>
> It was using the full range functions by mistake, output is now correct.
>
> cheers
>
> On 07/11/12 17:48, Burkhard Plaum wrote:
>> Hi,
>>
>> Am 07.11.2012 17:50, schrieb Al Crate:
>>> You can easily see this by running lqt_transcode on a prores mov.
>>>
>>>
>>>
>>> On 07/11/12 12:00, Al Crate wrote:
>>>> Hi, have been playing with the ProRes codec, seems to encode correctly,
>>>> but on decode the output is noticeably darker than the original.
>>>>
>>>> Has anyone else noticed this ?
>>>>
>>>> I've tried using ffmpeg directly and that works as expected. So I'm
>>>> suspecting libquicktimes conversion from YUV422P10 to RGB888.
>>>>
>> Well then you can fix the conversion or bypass the built in colormodel converter
>> completely. It's incredibly inefficient and only here for compatibility reasons.
>>
>> Too dark video is often related to a confusion of video scaled and JPEG scaled
>> YCbCr. Not sure if that applies here.
>>
>> Burkhard
>>
>> ------------------------------------------------------------------------------
>> LogMeIn Central: Instant, anywhere, Remote PC access and management.
>> Stay in control, update software, and manage PCs from one command center
>> Diagnose problems and improve visibility into emerging IT issues
>> Automate, monitor and manage. Do more in less time with Central
>> http://p.sf.net/sfu/logmein12331_d2d
>> _______________________________________________
>> Libquicktime-devel mailing list
>> Lib...@li...
>> https://lists.sourceforge.net/lists/listinfo/libquicktime-devel
>>
> ------------------------------------------------------------------------------
> Everyone hates slow websites. So do we.
> Make your web apps faster with AppDynamics
> Download AppDynamics Lite for free today:
> http://p.sf.net/sfu/appdyn_d2d_nov
> _______________________________________________
> Libquicktime-devel mailing list
> Lib...@li...
> https://lists.sourceforge.net/lists/listinfo/libquicktime-devel


-- 
Joseph Artsimovich
Senior C++ Applications Developer
MirriAd Ltd

Re: [Libquicktime-devel] Prores codec issues.

[Al C. <al...@dn...>]

The better solution was to fix the actual bug..

In the colorspace_macros.h

#define YUV_10_TO_RGB_24(y,u,v,r,g,b) \
   i_tmp=(yj_16_to_rgb * (y-0x40) + vj_16_to_r * (v-0x200))>>18;        \
   r = RECLIP_8(i_tmp);\
   i_tmp=(yj_16_to_rgb * (y-0x40) + uj_16_to_g * (u-0x200)+ vj_16_to_g * 
(v-0x200))>>18; \
   g = RECLIP_8(i_tmp);\
   i_tmp=(yj_16_to_rgb * (y-0x40) + uj_16_to_b * (u-0x200))>>18; \
   b = RECLIP_8(i_tmp);

should actually be

#define YUV_10_TO_RGB_24(y,u,v,r,g,b) \
   i_tmp=(y_16_to_rgb * (y-0x40) + v_16_to_r * (v-0x200))>>18;        \
   r = RECLIP_8(i_tmp);\
   i_tmp=(y_16_to_rgb * (y-0x40) + u_16_to_g * (u-0x200)+ v_16_to_g * 
(v-0x200))>>18; \
   g = RECLIP_8(i_tmp);\
   i_tmp=(y_16_to_rgb * (y-0x40) + u_16_to_b * (u-0x200))>>18; \
   b = RECLIP_8(i_tmp);

It was using the full range functions by mistake, output is now correct.

cheers

On 07/11/12 17:48, Burkhard Plaum wrote:
> Hi,
>
> Am 07.11.2012 17:50, schrieb Al Crate:
>> You can easily see this by running lqt_transcode on a prores mov.
>>
>>
>>
>> On 07/11/12 12:00, Al Crate wrote:
>>> Hi, have been playing with the ProRes codec, seems to encode correctly,
>>> but on decode the output is noticeably darker than the original.
>>>
>>> Has anyone else noticed this ?
>>>
>>> I've tried using ffmpeg directly and that works as expected. So I'm
>>> suspecting libquicktimes conversion from YUV422P10 to RGB888.
>>>
>
> Well then you can fix the conversion or bypass the built in colormodel converter
> completely. It's incredibly inefficient and only here for compatibility reasons.
>
> Too dark video is often related to a confusion of video scaled and JPEG scaled
> YCbCr. Not sure if that applies here.
>
> Burkhard
>
> ------------------------------------------------------------------------------
> LogMeIn Central: Instant, anywhere, Remote PC access and management.
> Stay in control, update software, and manage PCs from one command center
> Diagnose problems and improve visibility into emerging IT issues
> Automate, monitor and manage. Do more in less time with Central
> http://p.sf.net/sfu/logmein12331_d2d
> _______________________________________________
> Libquicktime-devel mailing list
> Lib...@li...
> https://lists.sourceforge.net/lists/listinfo/libquicktime-devel
>