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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
