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

[Michael Niedermayer]

Hi

On Wed, Jan 30, 2013 at 11:37:50AM -0800, Vignesh Venkatasubramanian wrote:
> 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).

This sounds like you could assume alpha_mode is always 1 and that
would be fine?


> 
> 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.

st->codec->pix_fmt should be a format with alpha if the decoder sets
it correctly and after avformat_find_stream_info()
this wouldnt work though if you want to detect alpha that occurs later.
pix_fmt is theoretically really the correct field for detecting alpha

If its important to know if a specific colorspace or alpha or anything
else can occur later in a video before that part is read then such
information would belong in a global header. (extradata in
ffmpeg terminology, codec private in matroska terminology)

Implicating the existence of alpha from max_block_additional_id
seems fragile to me. Consider that another block_additional_id
is added in the future for something else.

That said, if the design provides nothing better than
max_block_additional_id then that could also be exported as metadata as
you did, the problem seems more in the omission of such global
information paird with the need of it not so much in how its exportet.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130130/19ea1a4f/attachment.asc>