[FFmpeg-devel] patch for a new H.264 codec

[Moritz Barsnick]

Hi Yufei,
before providing large patches here, do read this mailing list and the
ffmpeg codebase for a while. It will help you to understand the
process, and to understand how ffmpeg's sources are organized.

I'm sure you missed this:
https://www.ffmpeg.org/developer.html

Read all of it thoroughly.

The content of this section:
https://www.ffmpeg.org/developer.html#Coding-Rules-1
especially comes to mind. Your code uses totally different formatting
than the rest of the ffmpeg codebase. You should take a look around as
see how others do it, and what that style guide says.

Apart from that: Everything that Nicolas wrote.

In addition this:

On Mon, Feb 25, 2019 at 19:49:43 +0000, Yufei He wrote:
> index c90f119..f70368b 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -11,6 +11,7 @@ version <next>:
>  - dhav demuxer
>  - PCM-DVD encoder
>  - GIF parser
> +- M264 encoder

Your patch is against an at least two months old version of ffmpeg git.
Please merge it to latest git HEAD and create a new patch. Your patch
won't apply currently, and therefore noone will bother to test it.

And even if I try to work around that, here's what happens:

LD      ffmpeg_g
/usr/bin/ld: libavcodec/libavcodec.a(m264enc.o): in function `ff_m264_encode_init':
/home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:98: undefined reference to `dlopen'
/usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:108: undefined reference to `dlsym'
/usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:109: undefined reference to `dlsym'
/usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:110: undefined reference to `dlsym'
/usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:111: undefined reference to `dlsym'
/usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:112: undefined reference to `dlsym'
/usr/bin/ld: libavcodec/libavcodec.a(m264enc.o):/home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:113: more undefined references to `dlsym' follow
/usr/bin/ld: libavcodec/libavcodec.a(m264enc.o): in function `ff_m264_encode_close':
/home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:264: undefined reference to `dlclose'
collect2: error: ld returned 1 exit status
make: *** [Makefile:108: ffmpeg_g] Error 1

You need to get your dependencies right.

> +   if(*got_output)        
> +   {
> +      if(decoded_frame->width == 0)
> +      {
> +         av_log(NULL, AV_LOG_DEBUG, "Frame parameters mismatch context %d,%d,%d != %d,%d,%d\n",
> +               decoded_frame->width,
> +               decoded_frame->height,
> +               decoded_frame->format,
> +               ist->dec_ctx->width,
> +               ist->dec_ctx->height,
> +               ist->dec_ctx->pix_fmt);
> +      }
> +   }

This is debug code and does not belong into a released codec.
Furthermore, ffmpeg provides av_log() functions.

> index 0000000..dc1852f
> --- /dev/null
> +++ b/libavcodec/.vscode/settings.json

Don't commit your local development environment's settings, please.

>  OBJS-$(CONFIG_DNXHD_DECODER)           += dnxhddec.o dnxhddata.o
>  OBJS-$(CONFIG_DNXHD_ENCODER)           += dnxhdenc.o dnxhddata.o
> +OBJS-$(CONFIG_M264_ENCODER)            += m264enc.o
> +OBJS-$(CONFIG_M264_DECODER)            += m264dec.o
>  OBJS-$(CONFIG_DOLBY_E_DECODER)         += dolby_e.o kbdwin.o
>  OBJS-$(CONFIG_DPX_DECODER)             += dpx.o

Do you realize that the rest of this list is in alphabetical order?

>  OBJS-$(CONFIG_VP9_SUPERFRAME_SPLIT_BSF)   += vp9_superframe_split_bsf.o
>  
> +
> +
> +
>  # thread libraries

Why do you add useless empty lines, and commit them?

>  extern AVCodec ff_dnxhd_encoder;
>  extern AVCodec ff_dnxhd_decoder;
> +extern AVCodec ff_m264_encoder;
> +extern AVCodec ff_m264_decoder;
>  extern AVCodec ff_dpx_encoder;
>  extern AVCodec ff_dpx_decoder;

Alphabetical order, again.

>      if (c)
> -        *opaque = (void*)(i + 1);
> -
> +      *opaque = (void*)(i + 1);
> +      
>      return c;

Useless change. And please don't ever leave whitespace at your line
endings, it won't be accepted. (Your IDE surely can fix this for you.)

> +#define FF_PROFILE_M264             0

What is this?

> +        .props     = AV_CODEC_PROP_LOSSY | AV_CODEC_PROP_REORDER,
> +        .profiles  = NULL_IF_CONFIG_SMALL(ff_h264_profiles),

Does the encoder even honor any of the profiles?

> +   #ifdef _WIN32
> +   av_log(avctx, AV_LOG_DEBUG, "_WIN32\n");
> +   #elif defined _WIN64
> +   av_log(avctx, AV_LOG_DEBUG, "_WIN64\n");
> +   #else
> +   av_log(avctx, AV_LOG_DEBUG, "linux\n");
> +   #endif

What for?

> +#ifdef _WIN32
> +   lib_handle = dlopen("mvM264Ffmpeg.dll", RTLD_LAZY);
> +#else   
> +   lib_handle = dlopen("libmvM264Ffmpeg.so", RTLD_LAZY);
> +#endif

I'm not sure this is acceptable within ffmpeg.

> +   if (!lib_handle)
> +   {
> +      av_log(avctx, AV_LOG_ERROR, "failed to load mvM264ffmpeg\n");
> +   }
> +
> +   m264_decoder =  av_mallocz(sizeof(M264Decoder));
> +
> +   m264_decoder->init_m264_decoder = dlsym(lib_handle, "m264_ffmpeg_decoder_init");
> +   m264_decoder->exit_m264_decoder = dlsym(lib_handle, "m264_ffmpeg_decoder_exit");
> +   m264_decoder->send_packet = dlsym(lib_handle, "m264_ffmpeg_decoder_send_packet");
> +   m264_decoder->receive_frame = dlsym(lib_handle, "m264_ffmpeg_decoder_receive_frame");
> +   m264_decoder->release_frame_buffer = dlsym(lib_handle, "m264_ffmpeg_decoder_release_frame_buffer");
> +   m264_decoder->lib_handle = lib_handle;

If it fails to load, you just continue? Honestly?

> +   switch (avctx->field_order)
> +   {
> +      case AV_FIELD_UNKNOWN: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_UNKNOWN \n");
> +         break;
> +      case AV_FIELD_PROGRESSIVE: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_PROGRESSIVE \n");
> +         break;
> +      case AV_FIELD_TT: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_TT \n");
> +         break;
> +      case AV_FIELD_BB: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_BB \n");
> +         break;
> +      case AV_FIELD_TB: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_TB \n");
> +         break;
> +      case AV_FIELD_BT: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_BT \n");
> +         break;
> +      default:
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is default \n");
> +         assert(false);
> +         break;
> +   }

Probably much too noisy. And coded way too complicated.

> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->width = %d\n", avctx->width);
> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->height = %d\n", avctx->height);
> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->framerate.num = %d\n", avctx->framerate.num);
> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->framerate.den = %d\n", avctx->framerate.den);

Probably much too noisy. And even if, could be in just one line.

> +   avctx->pix_fmt = AV_PIX_FMT_YUYV422;

That's all it supports?

> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_decode_close: 2 \n");

"2"?

> +++ b/libavcodec/m264enc.c
[...]
> +#include "m264enc.h"
[...]
> +#include "m264enc.h"

Twice? Please, come on. If even such simple things are done
incorrectly, how do you expect anyone to take the time to review the
rest of the code?

> +#ifdef _WIN32
> +   lib_handle = dlopen("mvM264Ffmpeg.dll", RTLD_LAZY);
> +#else   
> +   lib_handle = dlopen("libmvM264Ffmpeg.so", RTLD_LAZY);
> +#endif

I'm not sure this is acceptable within ffmpeg.

> +   printf("m264_encode_init_h264: avctx->width = %d\n", avctx->width);
> +   printf("m264_encode_init_h264: avctx->height = %d\n", avctx->height);
> +   printf("m264_encode_init_h264: avctx->framerate.num = %d\n", avctx->framerate.num);
> +   printf("m264_encode_init_h264: avctx->framerate.den = %d\n", avctx->framerate.den);
> +   printf("m264_encode_init_h264: avctx->gop_size = %d\n", avctx->gop_size);
> +   printf("m264_encode_init_h264: avctx->bit_rate = %" PRId64 "\n", avctx->bit_rate);

This is debug code and does not belong into a released codec.
Furthermore, ffmpeg provides av_log() functions.

> +      {
> +         result = sws_scale(m264_encoder->sw_context, (const uint8_t * const*)frame->data, frame->linesize, 0, frame->height, dst, dst_stride);
> +      }

I'm sure an eventual dependency to sws_scale must be registered in
configure.

> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_encode_close 1");

"1"?


And apart from this superficial review, there are probably quite a lot
of other quality issues.

Moritz