[FFmpeg-devel] [PATCH] do not set codec tag in raw video encoder

[Michael Niedermayer]

On Fri, Jun 11, 2010 at 03:01:42AM -0700, Baptiste Coudurier wrote:
> On 6/10/10 12:09 AM, Stefano Sabatini wrote:
>> On date Wednesday 2010-06-09 16:51:58 -0700, Baptiste Coudurier encoded:
>>> On 06/09/2010 02:57 PM, Stefano Sabatini wrote:
>>>> On date Wednesday 2010-06-09 20:38:15 +0200, Michael Niedermayer 
>>>> encoded:
>>>>> On Wed, Jun 09, 2010 at 02:47:21AM -0700, Baptiste Coudurier wrote:
>>>>>> On 6/5/10 3:35 PM, Baptiste Coudurier wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>> $subject, let the muxer choose the appropriate one depending on the
>>>>>>> format.
>>>>>>>
>>>>>>>
>>>>>>> rawenc_codec_tag.patch
>>>>>>>
>>>>>>>
>>>>>>> Index: libavcodec/rawenc.c
>>>>>>> ===================================================================
>>>>>>> --- libavcodec/rawenc.c	(revision 23498)
>>>>>>> +++ libavcodec/rawenc.c	(working copy)
>>>>>>> @@ -35,8 +35,6 @@
>>>>>>>        avctx->coded_frame->pict_type = FF_I_TYPE;
>>>>>>>        avctx->coded_frame->key_frame = 1;
>>>>>>>        avctx->bits_per_coded_sample =
>>>>>>> av_get_bits_per_pixel(&av_pix_fmt_descriptors[avctx->pix_fmt]);
>>>>>>> -    if(!avctx->codec_tag)
>>>>>>> -        avctx->codec_tag = 
>>>>>>> avcodec_pix_fmt_to_codec_tag(avctx->pix_fmt);
>>>>>>>        return 0;
>>>>>>>    }
>>>>>>>
>>>>>>
>>>>>> Any objection ?
>>>>>
>>>>> i suspect this might break muxing rawvideo in some containers
>>>>> also it does not seem to be completely in line with the api 
>>>>> documentation
>>>>> in avcodec.h for codec_tag
>>>>>
>>>>> iam not really objecting to change things but the docs would have to be
>>>>> clarified, muxers (especially nut) must be checked so they dont loose
>>>>> support for something that worked
>>>>
>>>> Indeed this change would break NUT rawvideo and the lavfi_pix_fmts
>>>> test.
>>>
>>> Did you check that you didn't break rawvideo in mov before applying
>>> your modifications ? The answer is no and it's broken now.
>>
>> Note that I limited myself to add tags, without to change the logic
>> behind, if adding entries broke it then there was already some problem
>> with the design. Anyway, what's for sure we need to list the MOV codec
>> tags in raw.c up *before* the NUT ones (as MOV tags are also valid NUT
>> tags, the reverse is not true), and remove the duplicated codec tags
>> entries, that should be enough to fix muxing.
>
> No that won't work. The codec tag must be chosen by the muxer,
> not the encoder. No encoder sets the codec tag right now,

libxvidff.c does and the muxer cant do it for it.


> why should rawvideo do it ?

because the pixel format is input to an encoder the output is described
by the codec_id, the problem is that we have 1 codec_id_rawvideo, had we one
for each pixel format this problem wouldnt arise but probably other problems
would.


>
> This patch is right IMHO, each container supporting rawvideo must do the 
> correct relation between pix_fmt <> codec tag

i dont disagree with this solution but we cant commit changes that break
some rawvideo formats in some (de)muxers. Changes toward that should be
done so nothing breaks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100611/d4a0b847/attachment.pgp>