[FFmpeg-devel] [PATCH 2/2] Copy VP8 BlockAdditional data to side_data of AVPacket

[Vignesh Venkatasubramanian]

On Tue, Jan 29, 2013 at 8:12 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Tue, Jan 29, 2013 at 02:56:48PM -0800, Vignesh Venkatasubramanian wrote:
>> Copy the data in BlockAdditional element of matroska container into
>> AVPacket's side data (with a new AVPacketSideDataType). This is being
>> done because alpha channel support for WebM is being added to chromium
>> and the per frame extra data needs to be demuxed and passed on to the
>> decoder. The design doc for alpha channel support in chromium can be
>> found here: http://goo.gl/wCP1y
>>
>> Signed-off-by: Vignesh Venkatasubramanian <vigneshv at google.com>
>> ---
>>  libavcodec/avcodec.h      |  5 +++++
>>  libavformat/matroskadec.c | 22 +++++++++++++++++++---
>>  2 files changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index ca7764a..871db5b 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -983,6 +983,11 @@ enum AVPacketSideDataType {
>>       * @endcode
>>       */
>>      AV_PKT_DATA_SUBTITLE_POSITION,
>> +
>> +    /**
>> +     * Alpha Channel data for VP8
>> +     */
>> +    AV_PKT_DATA_VP8_ALPHA,
>>  };
>>
>>  /**
>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 8b6b8d0..dc21795 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -1783,6 +1783,10 @@ static int matroska_read_header(AVFormatContext *s)
>>              if (track->video.stereo_mode && track->video.stereo_mode < MATROSKA_VIDEO_STEREO_MODE_COUNT)
>>                  av_dict_set(&st->metadata, "stereo_mode", ff_matroska_video_stereo_mode[track->video.stereo_mode], 0);
>>
>> +            /* export alpha mode flag as metadata tag */
>> +            if (track->max_block_additional_id && !strcmp(track->codec_id, "V_VP8"))
>> +                av_dict_set(&st->metadata, "alpha_mode", "1", 0);
>
> why is this needed ?
> metadata doesnt seem to be the proper place for such a flag
> it also may or may not be copied to the destination file on transcoding
> both could cause issues.
> consider a generic container that can perserve generic side data but
> the user chooses not to copy metadata (which should be optional)
> or that the metadata gets copied into a webm destination container
> automatically and the actual side data isnt copied or the encoder
> doesnt support encoding alpha.
> The result would be a webm with "alpha_mode" 1 but no alpha

The metadata indicates that the file could potentially have alpha data
in BlockAdditional. It doesn't guarantee presence of alpha. So in case
where the metadata is stripped, the video file will still be valid (it will
not be displayed as intended though). The case where the metadata
is set but there is no alpha is also fine because the metadata only
indicates that there could be alpha (there could be a file of 2 minutes
length with no alpha for the first minute).

In our specific case with chromium, we need some way to know
about the potential presence of alpha by looking just at the file headers.
If you have a better solution for our case, we'd love to hear it.

Also, I merely followed the "stereo_mode" flag that is already being set
as a metadata.

>
>> +
>>              /* if we have virtual track, mark the real tracks */
>>              for (j=0; j < track->operation.combine_planes.nb_elem; j++) {
>>                  char buf[32];
>> @@ -2087,7 +2091,8 @@ static int matroska_parse_frame(MatroskaDemuxContext *matroska,
>>                                  AVStream *st,
>>                                  uint8_t *data, int pkt_size,
>>                                  uint64_t timecode, uint64_t lace_duration,
>> -                                int64_t pos, int is_keyframe)
>> +                                int64_t pos, int is_keyframe,
>> +                                uint8_t *additional, int additional_size)
>>  {
>>      MatroskaTrackEncoding *encodings = track->encodings.elem;
>>      uint8_t *pkt_data = data;
>> @@ -2124,6 +2129,11 @@ static int matroska_parse_frame(MatroskaDemuxContext *matroska,
>>      pkt->flags = is_keyframe;
>>      pkt->stream_index = st->index;
>>
>
>> +    if (additional_size > 0 && !strcmp(track->codec_id, "V_VP8")) {
>
> this could use st->codec->codec_id instead, which would avoid the
> strcmp()
>
>
>
>> +        uint8_t *side_data = av_packet_new_side_data(pkt, AV_PKT_DATA_VP8_ALPHA, additional_size);
>> +        memcpy(side_data, additional, additional_size);
>
> missing check for malloc failure / side_data == NULL
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> There will always be a question for which you do not know the correct answer.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>



--
Vignesh V | Software Engineer | vigneshv at google.com | 650-861-1330