[Ffmpeg-devel] [PATCH] non conformance to 3GP format (3GP TS 26.244 v 6.5.0 2006/06)

[ffmpeg]

Hello M Niedermayer,

Thanks a lot for the time you spent studding my patch proposal.

This is my first patch delivery to the OpenSource Community. I apologize 
for the first glitches you notice.
I fully agree with all  your  remarks.

Do you want me to remake a patch proposal with all you comments 
integrated, or you have already done the job?

Thanks a lot for your help
Francois.

PS: should I send this mail also to the mailing list ? only to it ? or 
as I test only to you ( the reviewer)?.





Michael Niedermayer wrote:
> Hi
>
> On Wed, Jul 26, 2006 at 06:02:58PM +0200, ffmpeg wrote:
>   
>> Hello,
>>
>> I would like to submit for review (and maybe integration) the following 
>> patch:
>>
>>
>> Purpose:  Fix some non conformance to 3GP format  (3GP TS 26.244 v 6.5.0 
>> 2006/06)
>> Applies:   libavcodec/movenc.c {SVN 5720}, (test within trunk SVN 5726)
>> Testing:   
>>      - Regression test OK;
>>      - 3GP file check with some commercial 3gp analysis tools
>>      - play on QuickTime
>>      - play on HP OCMP Video 1.0
>> Authors:   Francois Draperi
>> Discussion:
>>    Some fields (like in 'samr' atom) are not set to 0; "3GP TS 26.244" 
>> enforces those values to be null.     
>>    The patch simply put 0 in the needed audio and video atom fields, for
>>    3GP files (and only them), and for AMR_NB mode (and only this codec).
>>    Maybe someone may also ask to restrict those changes to the '-strict 
>> very' mode, but is it useful ?
>>
>> Hope that help
>> Francois Draperi.
>>
>>     
>
> [...]
>   
>> +              (track->enc->codec_id == CODEC_ID_AMR_NB && track->mode != MODE_3GP) ; 
>>     
>
> trailing whitespace
>
>
>   
>> +   
>>      int version = track->mode == MODE_MOV &&
>>          (vbr ||
>>           track->enc->codec_id == CODEC_ID_PCM_S32LE ||
>>           track->enc->codec_id == CODEC_ID_PCM_S24LE);
>>  
>> +    
>> +
>>     
>
> cosmetical changes
>
>
> [...]
>   
>>  
>> +    printf("#### vbr is %d\n\n\n", vbr);
>>     
>
> printf is forbidden, this actually should not have compiled at all
>
>
>   
>>      if(vbr) {
>>          put_be16(pb, 0xfffe); /* compression ID (vbr)*/
>>      } else {
>> @@ -608,16 +613,25 @@
>>      put_be16(pb, 0); /* Reserved */
>>      put_be16(pb, 1); /* Data-reference index */
>>  
>> -    put_be16(pb, 0); /* Codec stream version */
>> -    put_be16(pb, 0); /* Codec stream revision (=0) */
>> -    put_tag(pb, "FFMP"); /* Vendor */
>> -    if(track->enc->codec_id == CODEC_ID_RAWVIDEO) {
>> +    /* FIX BEGIN as defined in  3GP TS 26.244 v 6.5.0 2006/06 ,for s263 atom, following  must be set to 0  [Francois Dr.]  */
>> +    if ( track->mode == MODE_3GP) {    
>> +      put_be32(pb, 0);
>>     
>
> indention in ffmpeg should be consitant, if a file uses 4-spaces, changes
> must use 4 spaces too unless you want to avoid unneeded whitespaces changes
> of existing code, but you didnt do that here ...
>
>
>   
>> +      put_be32(pb, 0);
>> +      put_be32(pb, 0);
>> +      put_be32(pb, 0);
>> +    } else {
>> +      put_be16(pb, 0); /* Codec stream version */
>> +      put_be16(pb, 0); /* Codec stream revision (=0) */
>>     
>
> the above 2 are correct for all cases so this is code duplication
>
>
>   
>> +      put_tag(pb, "FFMP"); /* Vendor */
>> +      if(track->enc->codec_id == CODEC_ID_RAWVIDEO) {
>>          put_be32(pb, 0); /* Temporal Quality */
>>          put_be32(pb, 0x400); /* Spatial Quality = lossless*/
>> -    } else {
>> -        put_be32(pb, 0x200); /* Temporal Quality = normal */
>> +      } else {
>> +	put_be32(pb, 0x200); /* Temporal Quality = normal */
>>     
>
> tabs are forbidden
>
> [...]
>