1029316 - [MediaEncoder] |VideoTrackEncoder::AppendVideoSegment| doesn't copy the value of VideoFrame::mForceBlack to mRawSegment.

Status: RESOLVED FIXED

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

Description

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

When I tried to disable a MediaSteamTrack, it doesn't work. After tracking the code, the root cause is the argument "VideoSements aSegment" of |VideoTrackEncoder::AppendVideoSegment| is not copy properly to mRawSegment.
There are at least two solutions for this bug.
1. Add one more argument in VideoSegment::AppendFrame for the value of VideoFrame::mForceBlack. 
2. Call VideoSegment::ReplaceWithDisabled again when the VideoFrame::mForceBlack is true.

I prefer first solution.

Comment 1

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1029316.patch v1.0

Add an argument in |VideoSegment::AppendFrame| for the information of bFroceBlack.

Comment 2

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8444934 [details] [diff] [review]
bug-1029316.patch v1.0

Shelly, would you might to give me some feedbacks?

Comment 3

[Shelly Lin [:shelly]]

Comment on attachment 8444934 [details] [diff] [review]
bug-1029316.patch v1.0

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

Thanks for the catch :)

::: content/media/VideoSegment.cpp
@@ +47,5 @@
>  void
>  VideoSegment::AppendFrame(already_AddRefed<Image>&& aImage,
>                            TrackTicks aDuration,
> +                          const IntSize& aIntrinsicSize,
> +                          bool bForceBlack)

Same here.

::: content/media/VideoSegment.h
@@ +98,5 @@
>  
> +  void AppendFrame(already_AddRefed<Image>&& aImage,
> +                   TrackTicks aDuration,
> +                   const IntSize& aIntrinsicSize,
> +                   bool bForceBlack = false);

s/bForceBlack/aForceBlack

Comment 4

[Benjamin Chen [:bechen]]

For the gtest testcase, if you only want to check the "force black segments" are handled by VP8TrackEncoder. You may overwrite the |void CreateMutedFrame(nsTArray<uint8_t>* aOutputBuffer)| in |TestVP8TrackEncoder| to see if the function is called by VP8TrackEncoder.

Comment 6

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1029316.patch v1.1

1. Address Shelly's comment.
2. Add a simple gtest for this patch.

Comment 7

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8450739 [details] [diff] [review]
bug-1029316.patch v1.1

Hi, Benjamin,
Could you please give me some feedback for the gtest part?

Comment 8

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

‘Try server result: https://tbpl.mozilla.org/?tree=Try&rev=ac33b5f95e9c

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #7)
> Comment on attachment 8450739 [details] [diff] [review]
> bug-1029316.patch v1.1
> 
> Hi, Benjamin,
> Could you please give me some feedback for the gtest part?

Comment 9

[Benjamin Chen [:bechen]]

Comment on attachment 8450739 [details] [diff] [review]
bug-1029316.patch v1.1

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

Overall looks good to me.

::: content/media/gtest/TestVideoSegment.cpp
@@ +3,5 @@
> +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "gtest/gtest.h"
> +#include "VideoSegment.h"
> +#include "ImageContainer.h"

Since we don't access the class members of mozilla::layers::Image, I guess we can just use forward declaration.

@@ +9,5 @@
> +using namespace mozilla;
> +
> +TEST(VideoSegment, TestAppendFrameForceBlack)
> +{
> +  nsRefPtr<mozilla::layers::Image> testImage = nullptr;

We don't need the "mozilla::" namespace here because you have already use it.

@@ +12,5 @@
> +{
> +  nsRefPtr<mozilla::layers::Image> testImage = nullptr;
> +
> +  VideoSegment segment;
> +  segment.AppendFrame(testImage.forget(), mozilla::TrackTicks(90000), mozilla::gfx::IntSize(640, 480), true);

80 characters per line.

@@ +24,5 @@
> +}
> +
> +TEST(VideoSegment, TestAppendFrameNotForceBlack)
> +{
> +  nsRefPtr<mozilla::layers::Image> testImage = nullptr;

Same as above.

@@ +27,5 @@
> +{
> +  nsRefPtr<mozilla::layers::Image> testImage = nullptr;
> +
> +  VideoSegment segment;
> +  segment.AppendFrame(testImage.forget(), mozilla::TrackTicks(90000), mozilla::gfx::IntSize(640, 480));

Same as above

Comment 10

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1029316.patch v1.2

Address Benjamin's comment.

Comment 11

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8450768 [details] [diff] [review]
bug-1029316.patch v1.2

Carry Shelly and Benjamin's f+.

Comment 12

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8450768 [details] [diff] [review]
bug-1029316.patch v1.2

roc, could you please review this patch? This patch fixes the bug of disabling MediaStreamTrack not working(bug 970778).
In this patch, I did below changes.
1. Add an argument in |VideoSegment::AppendFrame| for the information of bFroceBlack.
2. Add two simple gtest unit test for my modification.

Comment 13

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Try server result for this patch.
https://tbpl.mozilla.org/?tree=Try&rev=51a0058439bc

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #12)
> Comment on attachment 8450768 [details] [diff] [review]
> bug-1029316.patch v1.2
> 
> roc, could you please review this patch? This patch fixes the bug of
> disabling MediaStreamTrack not working(bug 970778).
> In this patch, I did below changes.
> 1. Add an argument in |VideoSegment::AppendFrame| for the information of
> bFroceBlack.
> 2. Add two simple gtest unit test for my modification.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e65266c480e4

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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