Hi Moritz It's the first time that I send patch to ffmpeg, for sure I don't know a lot of things for ffmpeg.
Thanks a lot for your detailed review. I'll read the doc and fix the issues you said. Yufei. On 02/25/2019 04:21 PM, Moritz Barsnick wrote: > 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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel