1320829 - WebM demuxer does not surface alpha

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Karolien]

To implement alpha in WebM-contained videos, the demuxer must first surface the alpha information.

Comment 1

[Karolien]

Attachment: Bug 1320829 - updated WebM demuxer to surface alpha information.

Review commit: https://reviewboard.mozilla.org/r/96098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96098/

Comment 2

[Karolien]

Original review granted on bug 944117 https://reviewboard.mozilla.org/r/94868/diff/4#index_header
Created this bug in order to leave other open as tracking bug for overall feature.

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8815114 [details]
Bug 1320829 - updated WebM demuxer to surface alpha information.

https://reviewboard.mozilla.org/r/96098/#review96250

::: dom/media/MediaData.h:621
(Diff revision 1)
>  
>  class MediaRawData : public MediaData {
>  public:
>    MediaRawData();
>    MediaRawData(const uint8_t* aData, size_t mSize);
> +  MediaRawData(const uint8_t* aData,

MediaRawData(const uint8_t* aData, size_t mSize,
               const uint8_t* aAlphaData, size_t aAlphaSize)
               
also fit in 80 columns

::: dom/media/MediaData.h:624
(Diff revision 1)
>    MediaRawData();
>    MediaRawData(const uint8_t* aData, size_t mSize);
> +  MediaRawData(const uint8_t* aData,
> +               size_t mSize,
> +               const uint8_t* aAlphaData,
> +               size_t mAlphaSize);

aAlphaSize

it was incorrectly set to mSize to, please fix that while you're at it.

::: dom/media/MediaData.cpp:399
(Diff revision 1)
>  }
>  
> +MediaRawData::MediaRawData(const uint8_t* aData,
> +                           size_t aSize,
> +                           const uint8_t* aAlphaData,
> +                           size_t aAlphaSize)

MediaRawData::MediaRawData(const uint8_t* aData, size_t aSize,
                           const uint8_t* aAlphaData, size_t aAlphaSize)
  : MediaData(RAW_DATA, 0)
  , mCrypto(mCryptoInternal)
  , mBuffer(aData, aSize)
  , mAlphaBuffer(aAlphaData, aAlphaSize)
{

this fit in 80 columns here (73)

::: dom/media/MediaInfo.h:326
(Diff revision 1)
>    // mImage may be cropped; currently only used with the WebM container.
>    // A negative width or height indicate that no cropping is to occur.
>    nsIntRect mImageRect;
> +
> +  // Indicates whether or not frames may contain alpha information.
> +  bool mAlphaPresent;

you could favour C++11 inline construction:

bool mAlphaPresent = false;

::: dom/media/webm/WebMDemuxer.cpp:636
(Diff revision 1)
> +    size_t alphaLength = 0;
> +    // Check packets for alpha information if file has declared alpha frames
> +    // may be present.
> +    if (mInfo.mVideo.HasAlpha()) {
> +      r = nestegg_packet_additional_data(holder->Packet(),
> +                                     1,

argument alignment is wrong

::: dom/media/webm/WebMDemuxer.cpp:642
(Diff revision 1)
> +                                     &alphaData,
> +                                     &alphaLength);
> +      if (r == -1) {
> +        WEBM_DEBUG(
> +          "nestegg_packet_additional_data failed to retrieve alpha data r=%d",
> +          r);

shouldn't this be a fatal error?

or is it possible that not all frames contain alpha plane?

if not, please return error and make this a fatal error.

Comment 4

[Jean-Yves Avenard [:jya]]

Please address the comments first.

Comment 5

[Karolien]

Comment on attachment 8815114 [details]
Bug 1320829 - updated WebM demuxer to surface alpha information.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96098/diff/1-2/

Comment 6

[Karolien]

Comment on attachment 8815114 [details]
Bug 1320829 - updated WebM demuxer to surface alpha information.

https://reviewboard.mozilla.org/r/96096/#review96284

Comment 7

[Karolien]

Comment on attachment 8815114 [details]
Bug 1320829 - updated WebM demuxer to surface alpha information.

https://reviewboard.mozilla.org/r/96098/#review96250

> MediaRawData(const uint8_t* aData, size_t mSize,
>                const uint8_t* aAlphaData, size_t aAlphaSize)
>                
> also fit in 80 columns

Hi sorry it won't fit given the format of the file so I've split the arguments over multiple lines.

> MediaRawData::MediaRawData(const uint8_t* aData, size_t aSize,
>                            const uint8_t* aAlphaData, size_t aAlphaSize)
>   : MediaData(RAW_DATA, 0)
>   , mCrypto(mCryptoInternal)
>   , mBuffer(aData, aSize)
>   , mAlphaBuffer(aAlphaData, aAlphaSize)
> {
> 
> this fit in 80 columns here (73)

Hi are you referring to the signature? I think it is 103 characters so it won't fit without being split.

> shouldn't this be a fatal error?
> 
> or is it possible that not all frames contain alpha plane?
> 
> if not, please return error and make this a fatal error.

As per the spec, it is possible that the header declares alpha is present and that there are frames that do not contain alpha, so it should not be a fatal error.

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8815114 [details]
Bug 1320829 - updated WebM demuxer to surface alpha information.

https://reviewboard.mozilla.org/r/96098/#review96250

> Hi are you referring to the signature? I think it is 103 characters so it won't fit without being split.

Seems that reviewboard didn't include my CR..

so let's do it again.

I modified the file locally, and it's definitely not over 80 lines.

MediaRawData::MediaRawData(const uint8_t aData, size_t aSize,
                           const uint8_t aAlphaData, size_t aAlphaSize)
  : MediaData(RAW_DATA, 0)
  , mCrypto(mCryptoInternal)
  , mBuffer(aData, aSize)
  , mAlphaBuffer(aAlphaData, aAlphaSize)
{


same in the header.

Comment 9

[Karolien]

Comment on attachment 8815114 [details]
Bug 1320829 - updated WebM demuxer to surface alpha information.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96098/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c599df1589cc
updated WebM demuxer to surface alpha information. r=jya

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c599df1589cc