[FFmpeg-devel] [PATCH v2 25/36] vaapi_encode_mjpeg: Use CBS to store parameters and write headers

[Mark Thompson]

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