On Mon, Jul 11, 2016 at 5:55 PM, James Zern <jzern-at-google....@ffmpeg.org> wrote: > On Thu, Jul 7, 2016 at 11:43 AM, Vignesh Venkatasubramanian > <vigneshv-at-google....@ffmpeg.org> wrote: >> VPx (VP8/VP9) alpha encoding has been part of FFmpeg. Now, add the >> ability to decode such files with alpha channel. >> >> Signed-off-by: Vignesh Venkatasubramanian <vigne...@google.com> >> --- >> libavcodec/libvpxdec.c | 111 ++++++++++++++++++++++++++++------- >> tests/fate/vpx.mak | 3 + >> tests/ref/fate/vp8-alpha-decode | 125 >> ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 219 insertions(+), 20 deletions(-) >> create mode 100644 tests/ref/fate/vp8-alpha-decode >> >> [...] >> +static int vpx_codec_dec_init_wrapper(AVCodecContext *avctx, >> > > maybe just decoder/dec_init? >
have re-used vpx_init. >> [...] >> +static av_cold int vpx_init(AVCodecContext *avctx, >> + const struct vpx_codec_iface *iface) >> +{ >> + VP8Context *ctx = avctx->priv_data; > > just leave this extraction to the init wrapper. done. > >> + return vpx_codec_dec_init_wrapper(avctx, ctx, iface, 0); > >> [...] >> +static int vpx_codec_decode_wrapper(AVCodecContext *avctx, >> > > maybe just decode / decode_frame? done. > >> [...] >> + side_data = av_packet_get_side_data(avpkt, >> + >> AV_PKT_DATA_MATROSKA_BLOCKADDITIONAL, >> + &side_data_size); >> + if (side_data_size > 1) { >> + uint64_t additional_id = AV_RB64(side_data); >> > > could be const. done. > >> + side_data += 8; >> + side_data_size -= 8; >> > > this adjustment could be within the if(). it's a bit better future-proof'ed this way. if we need to process another additional_id later, it can just be an else-if. > >> [...] >> - av_image_copy(picture->data, picture->linesize, (const uint8_t >> **)img->planes, >> - img->stride, avctx->pix_fmt, img->d_w, img->d_h); >> + >> + planes[0] = img->planes[VPX_PLANE_Y]; >> + planes[1] = img->planes[VPX_PLANE_U]; >> + planes[2] = img->planes[VPX_PLANE_V]; >> + planes[3] = >> + ctx->has_alpha_channel ? img_alpha->planes[VPX_PLANE_Y] : NULL; >> + linesizes[0] = img->stride[VPX_PLANE_Y]; >> + linesizes[1] = img->stride[VPX_PLANE_U]; >> + linesizes[2] = img->stride[VPX_PLANE_V]; >> + linesizes[3] = >> + ctx->has_alpha_channel ? img_alpha->stride[VPX_PLANE_Y] : 0; >> + av_image_copy(picture->data, picture->linesize, (const >> uint8_t**)planes, >> + linesizes, avctx->pix_fmt, img->d_w, img->d_h); >> > > couldn't this just be 1 additional av_image_copy_plane()? av_image_copy does some width computation [1] before calling av_image_copy_plane. i didn't want to duplicate that computation here. it just seemed cleaner this way. please let me know if you strongly feel otherwise and i can change this. [1] https://github.com/FFmpeg/FFmpeg/blob/eae2d89bf715bc3edff478174b43e1f388e768bf/libavutil/imgutils.c#L326 > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel -- Vignesh _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel