On 15/06/18 02:37, Xiang, Haihao wrote: > On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote: >> Also adds greyscale, 4:2:2, 4:4:4 and RGB support. >> --- >> configure | 2 +- >> doc/encoders.texi | 17 +- >> libavcodec/vaapi_encode_mjpeg.c | 529 +++++++++++++++++++++++++------------ >> --- >> 3 files changed, 347 insertions(+), 201 deletions(-) >> >> ... >> diff --git a/libavcodec/vaapi_encode_mjpeg.c >> b/libavcodec/vaapi_encode_mjpeg.c >> index f76645425a..2f79070e58 100644 >> --- a/libavcodec/vaapi_encode_mjpeg.c >> +++ b/libavcodec/vaapi_encode_mjpeg.c >> ... >> +static int vaapi_encode_mjpeg_write_image_header(AVCodecContext *avctx, >> + VAAPIEncodePicture *pic, >> + VAAPIEncodeSlice *slice, >> + char *data, size_t >> *data_len) >> { >> - int i, mt; >> - >> - ++src_lengths; >> + VAAPIEncodeMJPEGContext *priv = avctx->priv_data; >> + CodedBitstreamFragment *frag = &priv->current_fragment; >> + int err; >> + >> + if (priv->jfif) { >> + err = ff_cbs_insert_unit_content(priv->cbc, frag, -1, >> + JPEG_MARKER_APPN + 0, >> + &priv->jfif_header, NULL); >> + if (err < 0) >> + goto fail; >> + } >> >> - mt = 0; >> - for (i = 0; i < 16; i++) >> - mt += (dst_lengths[i] = src_lengths[i]); >> + err = ff_cbs_insert_unit_content(priv->cbc, frag, -1, >> + JPEG_MARKER_DQT, >> + &priv->quant_tables, NULL); >> + if (err < 0) >> + goto fail; >> + >> + err = ff_cbs_insert_unit_content(priv->cbc, frag, -1, >> + JPEG_MARKER_SOF0, >> + &priv->frame_header, NULL); >> + if (err < 0) >> + goto fail; >> + >> + if (priv->huffman) { >> + err = ff_cbs_insert_unit_content(priv->cbc, frag, -1, >> + JPEG_MARKER_DHT, >> + &priv->huffman_tables, NULL); >> + if (err < 0) >> + goto fail; >> + } >> >> - for (i = 0; i < mt; i++) >> - dst_values[i] = src_values[i]; >> -} >> + err = ff_cbs_insert_unit_content(priv->cbc, frag, -1, >> + JPEG_MARKER_SOS, >> + &priv->scan, NULL); >> + if (err < 0) >> + goto fail; >> >> -static av_cold void vaapi_encode_mjpeg_init_tables(AVCodecContext *avctx) >> -{ >> - VAAPIEncodeMJPEGContext *priv = avctx->priv_data; >> - VAQMatrixBufferJPEG *quant = &priv->quant_tables; >> - VAHuffmanTableBufferJPEGBaseline *huff = &priv->huffman_tables; >> - int i; >> - >> - quant->load_lum_quantiser_matrix = 1; >> - quant->load_chroma_quantiser_matrix = 1; >> + err = ff_cbs_write_fragment_data(priv->cbc, frag); >> + if (err < 0) { >> + av_log(avctx, AV_LOG_ERROR, "Failed to write image header.\n"); >> + return err; > > Should it be 'goto fail' ? Some new units have been inserted to the fragment > so > ff_cbs_fragment_uninit() should be called to release resources.
Yep, fixed. >> + } >> >> - for (i = 0; i < 64; i++) { >> - quant->lum_quantiser_matrix[i] = >> - vaapi_encode_mjpeg_quant_luminance[i]; >> - quant->chroma_quantiser_matrix[i] = >> - vaapi_encode_mjpeg_quant_chrominance[i]; >> + if (*data_len < 8 * frag->data_size) { >> + av_log(avctx, AV_LOG_ERROR, "Image header too large: " >> + "%zu < %zu.\n", *data_len, 8 * frag->data_size); >> + err = AVERROR(ENOSPC); >> + goto fail; > > Could you change the last parameter name of this function to bit_len or > bit_data_len? I think it is more readable and user don't need to think why > frag- >> data_size is multiplied by 8. I don't think I agree? It matches the naming and style used in all of the other codecs in these write-header functions - compare for example <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/vaapi_encode_h264.c;h=905c50760e2ba88f809b1e1a2e6059fce7134d53;hb=HEAD#l115>. >> } >> >> - huff->load_huffman_table[0] = 1; >> - vaapi_encode_mjpeg_copy_huffman(huff->huffman_table[0].num_dc_codes, >> - huff->huffman_table[0].dc_values, >> - avpriv_mjpeg_bits_dc_luminance, >> - avpriv_mjpeg_val_dc); >> - vaapi_encode_mjpeg_copy_huffman(huff->huffman_table[0].num_ac_codes, >> - huff->huffman_table[0].ac_values, >> - avpriv_mjpeg_bits_ac_luminance, >> - avpriv_mjpeg_val_ac_luminance); >> - memset(huff->huffman_table[0].pad, 0, sizeof(huff- >>> huffman_table[0].pad)); >> - >> - huff->load_huffman_table[1] = 1; >> - vaapi_encode_mjpeg_copy_huffman(huff->huffman_table[1].num_dc_codes, >> - huff->huffman_table[1].dc_values, >> - avpriv_mjpeg_bits_dc_chrominance, >> - avpriv_mjpeg_val_dc); >> - vaapi_encode_mjpeg_copy_huffman(huff->huffman_table[1].num_ac_codes, >> - huff->huffman_table[1].ac_values, >> - avpriv_mjpeg_bits_ac_chrominance, >> - avpriv_mjpeg_val_ac_chrominance); >> - memset(huff->huffman_table[1].pad, 0, sizeof(huff- >>> huffman_table[1].pad)); >> -} >> + // Remove the EOI at the end of the fragment. >> + memcpy(data, frag->data, frag->data_size - 2); >> + *data_len = 8 * (frag->data_size - 2); >> >> -static void vaapi_encode_mjpeg_write_marker(PutBitContext *pbc, int marker) >> -{ >> - put_bits(pbc, 8, 0xff); >> - put_bits(pbc, 8, marker); >> + err = 0; >> +fail: >> + ff_cbs_fragment_uninit(priv->cbc, frag); >> + return err; >> } >> ... Thanks, - Mark _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel