Closed Bug 1306521 Opened 3 years ago Closed 3 years ago

Handle VP9 colorspace BT.709 on BasicCompositor

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(4 files, 11 obsolete files)

27.63 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.28 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
4.94 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
23.96 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Split from Bug 1210357
Assignee: nobody → sotaro.ikeda.g
Blocks: 1210357
Attachment #8796386 - Attachment description: patch part 2 - Add YUVColorSpace handling to MediaData → patch part 3 - Add YUVColorSpace handling to MediaData
Attachment #8796386 - Flags: review?(jwwang)
Attachment #8796387 - Flags: review?(jyavenard)
Attachment #8796385 - Flags: review?(jmuizelaar)
Attachment #8796384 - Attachment is obsolete: true
Attachment #8796394 - Flags: review?(nical.bugzilla)
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?
(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.
Bug 1305907 is a bug to investigate to change default color space.
Attachment #8796386 - Flags: review?(jwwang) → review+
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..
Attachment #8796387 - Flags: review?(jyavenard) → review-
Attachment #8796394 - Flags: review?(nical.bugzilla) → review+
(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.
Apply the comment.
Attachment #8796387 - Attachment is obsolete: true
Attachment #8797878 - Flags: review?(jyavenard)
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.
Attachment #8797878 - Flags: review?(jyavenard) → review-
Apply the comment.
Attachment #8797878 - Attachment is obsolete: true
Attachment #8797901 - Attachment is obsolete: true
Attachment #8797911 - Flags: review?(jyavenard)
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.
Attachment #8797911 - Flags: review?(jyavenard) → review+
Apply the comment.
Attachment #8797911 - Attachment is obsolete: true
Attachment #8797925 - Flags: review+
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
Add AVCOL_SPC_SMPTE170M handling.
Attachment #8797925 - Attachment is obsolete: true
Attachment #8798299 - Flags: review+
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
Thanks for the advice. I will update the patch.
Apply the comment.
Attachment #8798299 - Attachment is obsolete: true
Attachment #8798313 - Flags: review+
Rebased.
Attachment #8796394 - Attachment is obsolete: true
Attachment #8799633 - Flags: review+
Rebased.
Attachment #8799633 - Attachment is obsolete: true
Attachment #8799637 - Flags: review+
Attachment #8799637 - Attachment is obsolete: true
Attachment #8799648 - Flags: review+
Attachment #8799648 - Attachment is obsolete: true
Attachment #8799649 - Flags: review+
Attachment #8796385 - Flags: review?(jmuizelaar) → review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b520bbe1d52
Handle VP9 colorspace BT.709 on BasicCompositor r=nical,jwwang,jya,jrmuizel
https://hg.mozilla.org/mozilla-central/rev/2b520bbe1d52
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1309802
You need to log in before you can comment on or make changes to this bug.