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

[jianwen chen]

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.



>
>> Diego
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at mplayerhq.hu
>> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-ffmpeg-svn-r24248-for-libxavs-update5.patch
Type: application/octet-stream
Size: 16474 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100721/82c44c8b/attachment.obj>