[FFmpeg-devel] [PATCH] adding xavs encoding support

Stefan Gehrer stefan.gehrer
Tue Jul 27 09:22:23 CEST 2010


On 07/21/2010 10:14 AM, jianwen chen wrote:
> On Sun, Jul 18, 2010 at 10:36 PM, jianwen chen<jianwen.chen.video at gmail.com
>> wrote:
>
>> On Sun, Jul 18, 2010 at 4:04 PM, Diego Biurrun<diego at biurrun.de>  wrote:
>>
>>> On Sun, Jul 18, 2010 at 02:47:46PM +0800, jianwen chen wrote:
>>>> On Sat, Jul 17, 2010 at 6:55 PM, Diego Biurrun<diego at biurrun.de>
>>> wrote:
>>>>
>>>>>> --- libavcodec/libxavs.c      (revision 0)
>>>>>> +++ libavcodec/libxavs.c      (revision 0)
>>>>> +#include "avcodec.h"
>>>>>> +#include<xavs.h>
>>>>>> +#include<math.h>
>>>>>> +#include<stdio.h>
>>>>>> +#include<stdlib.h>
>>>>>> +#include<string.h>
>>>>>> +#define END_OF_STREAM 0x001
>>>>>
>>>>> Place system headers before local headers.
>>>>
>>>>      OK,  however, I also refer to the libx264.c with the same order for
>>> the
>>>> including headers.
>>>
>>> That file does it wrong then.
>>>
>>
>> Ok. I change to this order,
>> +#include<stdio.h>
>> +#include<stdlib.h>
>> +#include<string.h>
>> +#include<math.h>
>> +#include<stdint.h>
>> +#include<xavs.h>
>> +#include "avcodec.h"
>>
>>
>>>
>>>> I can change the order as following
>>>>
>>>>> +#include<stdio.h>
>>>>> +#include<stdlib.h>
>>>>> +#include<string.h>
>>>>> +#include<math.h>
>>>>
>>>>> +#include "avcodec.h"
>>>>> +#include<xavs.h>
>>>
>>> Better, but only slightly, you still place xavs.h after avcodec.h.
>>>
>>>> --- libavcodec/libxavs.c      (revision 0)
>>>> +++ libavcodec/libxavs.c      (revision 0)
>>>> @@ -0,0 +1,355 @@
>>>> +
>>>> +#include<stdio.h>
>>>> +#include<stdlib.h>
>>>> +#include<string.h>
>>>> +#include<math.h>
>>>> +
>>>> +#include "avcodec.h"
>>>> +#include<xavs.h>
>>>
>>> Place all system headers before local headers.
>>>
>>>> +/* Analyze i8x8 (requires 8x8 transform) */
>>>> +#define XAVS_PART_I8X8 0x002
>>>> +/* Analyze p16x8, p8x16 and p8x8 */
>>>> +#define XAVS_PART_P8X8 0x010
>>>> +/* Analyze b16x8, b*/
>>>> +#define XAVS_PART_B8X8 0x100
>>>
>>> more readable:
>>>
>>>   #define XAVS_PART_I8X8 0x002   /* Analyze i8x8 (requires 8x8 transform)
>>> */
>>>   #define XAVS_PART_P8X8 0x010   /* Analyze p16x8, p8x16 and p8x8 */
>>>   #define XAVS_PART_B8X8 0x100   /* Analyze b16x8, b */
>>>
>>    done;
>>
>>
>>>> +    for (i = 0; i<  nnal; i++) {
>>>> +        /* Don't put the SEI in extradata. */
>>>> +        if (skip_sei&&  nals[i].i_type == NAL_SEI) {
>>>> +            x4->sei = av_malloc( 5 + nals[i].i_payload * 4 / 3 );
>>>> +            if(xavs_nal_encode(x4->sei,&x4->sei_size, 1, nals + i)<
>>> 0)
>>>
>>> if (
>>>
>>>> +    if( !bufsize&&  !frame&&  !(x4->end_of_stream) ){
>>>
>>> Inconsistent formatting; you again miss the space after the if and put
>>> spaces inside the parentheses.
>>>
>>>   Yes. I modified.
>>
>>
>>>> +    /*Amanda thinks there is no IDR frame in AVS 1.0.Sequence header is
>>> used as a flag*/
>>>
>>> long line
>>>
>>>
>>   Shorten the comment.
>>
>>
>>>> +    x4->params.pf_log               = XAVS_log;
>>>> +    x4->params.p_log_private        = avctx;
>>>> +//  x4->params.i_frame_total        = max_frames[CODEC_TYPE_VIDEO];
>>>
>>> Why is this commented out but still present?
>>>
>>>
>> Remove this commented sentence. I list this sentence here to make a mark
>> for future improvement.
>>   We can remove it now.
>>
>>
>>>> +    /*AMANDA TAG: AVS doesn't allow B picture as reference and the max
>>> allowed number of B is 2*/
>>>
>>> see above
>>>
>>> And what is AMANDA?
>>>
>>>
>>
>>    OK, I delete the AMANDA from the file. AMANDA is a pretty girl who has
>> worked on this code before.
>>
>>
>>>> +    /*AMANDA TAG: Amanda thinks this is only used for counting the
>>> fps*/
>>>
>>> Here and in other places: space after /* and before */ would aid
>>> readability.
>>>
>>> Good point, I have checked all the /* */ with your comments.
>>
>>
>>>> +    if (avctx->me_method == ME_EPZS){
>>>> +        x4->params.analyse.i_me_method = XAVS_ME_DIA;
>>>> +    }
>>>> +    else if (avctx->me_method == ME_HEX){
>>>> +        x4->params.analyse.i_me_method = XAVS_ME_HEX;
>>>> +    }
>>>> +    else if (avctx->me_method == ME_UMH){
>>>> +        x4->params.analyse.i_me_method = XAVS_ME_UMH;
>>>> +    }
>>>> +    else if (avctx->me_method == ME_FULL){
>>>> +        x4->params.analyse.i_me_method = XAVS_ME_ESA;
>>>> +    }
>>>> +    else if (avctx->me_method == ME_TESA){
>>>> +        x4->params.analyse.i_me_method = XAVS_ME_TESA;
>>>> +    }
>>>> +    else{
>>>> +        x4->params.analyse.i_me_method = XAVS_ME_HEX;
>>>> +    }
>>>
>>> This could be a switch IMO.
>>>
>>
>> Ok, I change this to switch ..case...
>>
>> The updated patch is attached for checking.  Thanks for your help.
>>
>>
>
>    I have modified all the problems you mentioned, the patch is attached.
>   Please review and check.

I applied it after adding some changes to Changelog and documentation
and I added myself as maintainer of the new file libavcodec/libxavs.c

Stefan



More information about the ffmpeg-devel mailing list