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

[jianwen chen]

On Tue, Jul 27, 2010 at 3:22 PM, Stefan Gehrer <stefan.gehrer at gmx.de> wrote:

> 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
>
>
   Ok, Thanks.
   I will try to improve the performance of XAVS in the near 3 months.
   And for cavs video, there is a standard file format .asm, I have all the
codes now.
   I will try to add this file format support  to ffmpeg.  I will do it
several months later.
   Thanks again for your help.

Stefan
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>