1306521 - Handle VP9 colorspace BT.709 on BasicCompositor

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Sotaro Ikeda [:sotaro]]

Split from Bug 1210357

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add YUVColorSpace handling to gfx/layers

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add YUVColorSpace handling to gfx/ycbcr

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Add YUVColorSpace handling to MediaData

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Add YUVColorSpace handling to FFmpegVideoDecoder

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add YUVColorSpace handling to gfx/layers

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8796386 [details] [diff] [review]
patch part 3 - Add YUVColorSpace handling to MediaData

Review of attachment 8796386 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaData.h
@@ +443,5 @@
>        uint32_t mSkip;
>      };
>  
>      Plane mPlanes[3];
> +    YUVColorSpace mYUVColorSpace = YUVColorSpace::BT601;

Where is YUVColorSpace defined? Why do you choose YUVColorSpace::BT601 to be the default value?

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #6)
> Comment on attachment 8796386 [details] [diff] [review]
> patch part 3 - Add YUVColorSpace handling to MediaData
> 
> Review of attachment 8796386 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaData.h
> @@ +443,5 @@
> >        uint32_t mSkip;
> >      };
> >  
> >      Plane mPlanes[3];
> > +    YUVColorSpace mYUVColorSpace = YUVColorSpace::BT601;
> 
> Where is YUVColorSpace defined? Why do you choose YUVColorSpace::BT601 to be
> the default value?

YUVColorSpace is added to gfx/layers/ImageTypes.h by attachment 8796394 [details] [diff] [review].

Current gecko uses only BT601. Therefore use YUVColorSpace::BT601 as the default value for now as not to break current yuv rendering unexpectedly by this bug.

Comment 8

[Sotaro Ikeda [:sotaro]]

Bug 1305907 is a bug to investigate to change default color space.

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8796387 [details] [diff] [review]
patch part 4 - Add YUVColorSpace handling to FFmpegVideoDecoder

Review of attachment 8796387 [details] [diff] [review]:
-----------------------------------------------------------------

From the FFmpeg doc:
"YUV colorspace type.

It must be accessed using av_frame_get_colorspace() and av_frame_set_colorspace().

encoding: Set by user
decoding: Set by libavcodec
Definition at line 431 of file frame.h."

That is because the AVFrame struct can't be allocated on the stack, address offset of each members can change depending on the version. And there's no compatibility between LibAV and FFmpeg. 

The only place you could do it safely is with our internal ffvp9 copy. 

You'll need to amend the runtime linker to resolve the appropriate symbols. Make it optional, so you only need to check if the symbol exists within the code..

Bonus points: why limit this to just libavcodec 57? It exists on earlier version. Seems to have been introduced in ffmpeg though we probably don't care for those..

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Jean-Yves Avenard [:jya] from comment #9)
> 
> Bonus points: why limit this to just libavcodec 57? It exists on earlier
> version. Seems to have been introduced in ffmpeg though we probably don't
> care for those..

It was because that a compiled was failed without it since AVFrame::colorspace was defined only on libavcodec 57. AVCodecContext::colorspace was defined on all versions though.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Add YUVColorSpace handling to FFmpegVideoDecoder

Apply the comment.

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8797878 [details] [diff] [review]
patch part 4 - Add YUVColorSpace handling to FFmpegVideoDecoder

Review of attachment 8797878 [details] [diff] [review]:
-----------------------------------------------------------------

It's good overall, much simpler that way, but one downside is that now all Libav libs have been blacklisted.

::: dom/media/platforms/ffmpeg/FFmpegLibWrapper.cpp
@@ +114,5 @@
>    AV_FUNC(avcodec_free_frame, AV_FUNC_54)
>    AV_FUNC(av_log_set_level, AV_FUNC_AVUTIL_ALL)
>    AV_FUNC(av_malloc, AV_FUNC_AVUTIL_ALL)
>    AV_FUNC(av_freep, AV_FUNC_AVUTIL_ALL)
> +  AV_FUNC(av_frame_get_colorspace, AV_FUNC_AVUTIL_ALL)

you can't use the AV_FUNC macro, as it will abort if the symbol can't be resolved. so as this symbol only exists in FFmpeg 2.2 and later, it will mean that we would no longer be able to use any Libav libs or earlier version of FFmpeg.

I guess we need to handle this symbol individually, differently to the other. Or add a new argument to the macro indicating that the symbol is optional.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Add YUVColorSpace handling to FFmpegVideoDecoder

Apply the comment.

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Add YUVColorSpace handling to FFmpegVideoDecoder

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8797911 [details] [diff] [review]
patch part 4 - Add YUVColorSpace handling to FFmpegVideoDecoder

Review of attachment 8797911 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with changes applied.

::: dom/media/platforms/ffmpeg/FFmpegLibWrapper.cpp
@@ +96,5 @@
>    } else {                                                                     \
>      func = (decltype(func))nullptr;                                            \
>    }
> +
> +#define AV_FUNC_OPTION(func, ver)                                             \

Please have AV_FUNC macro use this one... No need for complex macro duplication, they are painful enough to maintain..

@@ +97,5 @@
>      func = (decltype(func))nullptr;                                            \
>    }
> +
> +#define AV_FUNC_OPTION(func, ver)                                             \
> +  if ((ver) & version) {                                                      \

nit, the \ character here isn't aligned with the others.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Add YUVColorSpace handling to FFmpegVideoDecoder

Apply the comment.

Comment 17

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35df2740171b6412ab9484dc557ab202399fb8d3

Comment 18

[Sotaro Ikeda [:sotaro]]

It seems better to handle AVCOL_SPC_SMPTE170M as BT601 like chromium.
  https://chromium.googlesource.com/chromium/src/media/+/master/ffmpeg/ffmpeg_common.cc#714

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Add YUVColorSpace handling to FFmpegVideoDecoder

Add AVCOL_SPC_SMPTE170M handling.

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8798299 [details] [diff] [review]
patch part 4 - Add YUVColorSpace handling to FFmpegVideoDecoder

Review of attachment 8798299 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp
@@ +300,5 @@
>    } else {
>      b.mPlanes[1].mWidth = b.mPlanes[2].mWidth = (mFrame->width + 1) >> 1;
>      b.mPlanes[1].mHeight = b.mPlanes[2].mHeight = (mFrame->height + 1) >> 1;
>    }
> +  if (mLib->av_frame_get_colorspace) {

switch/case would likely be better. I can already imagine that we're going to have to add more cases to this

Comment 21

[Sotaro Ikeda [:sotaro]]

Thanks for the advice. I will update the patch.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Add YUVColorSpace handling to FFmpegVideoDecoder

Apply the comment.

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add YUVColorSpace handling to gfx/layers

Rebased.

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add YUVColorSpace handling to gfx/layers

Rebased.

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add YUVColorSpace handling to gfx/layers

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add YUVColorSpace handling to gfx/layers

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b520bbe1d52
Handle VP9 colorspace BT.709 on BasicCompositor r=nical,jwwang,jya,jrmuizel

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b520bbe1d52