919979 - Crash when calling WebrtcVideoConduit::SendVideoFrame() with very small resolution

Status: RESOLVED FIXED

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

Description

[Shian-Yow Wu [:swu]]

Attachment: GDB back trace log

In mediaconduit_unittest.cpp, if we call mVideoSession->SendVideoFrame() and pass 4x2 or 2x4 as input resolution, it may crash after test cases finished.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

tim - is this fixed in a newer release?

Comment 2

[Timothy B. Terriberry (:derf)]

(In reply to Randell Jesup [:jesup] from comment #1)
> tim - is this fixed in a newer release?

I have no idea. I don't remember seeing any issues on the subject.

Comment 3

[Shian-Yow Wu [:swu]]

With debug version, it asserted at below location.

Breakpoint 3, webrtc::VP8EncoderImpl::GetEncodedPartitions (this=0x7fffe67021a0, input_image=...)
    at /home/sywu/work/mozilla-central/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:448
448	        assert(encoded_image_._length <= encoded_image_._size);

The values are:
  encoded_image_._length = 30
  encoded_image_._size = 6

The resolution been tested is 2x4, and we allocate the video frame length for 6 bytes (width*height*3/2) bytes.

Comment 4

[Shian-Yow Wu [:swu]]

(In reply to Shian-Yow Wu [:shianyow] from comment #3)
> The resolution been tested is 2x4, and we allocate the video frame length
> for 6 bytes (width*height*3/2) bytes.

To correct, it is 12 bytes for resolution 2x4.

Comment 5

[Shian-Yow Wu [:swu]]

Sorry, to clarify again.
The resolution been tested was 2x2 with frame legnth 6 allocated in the above example.

Comment 6

[Shian-Yow Wu [:swu]]

It seems a VP8 encoder needs at least 31 bytes(first time 30 and second time 1) to process the frame.

However, we cannot just allocate and pass a larger frame length (ex: width*height*3/2 + 100), because there is a buffer size check at:
dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_capture/video_capture_impl.cc?from=video_capture_impl.cc#l254

Is it a required check?  Or can we modify the CalcBufferSize() calculation in order to reserve more buffer?

Comment 7

[Timothy B. Terriberry (:derf)]

(In reply to Shian-Yow Wu [:shianyow] from comment #6)
> However, we cannot just allocate and pass a larger frame length (ex:
> width*height*3/2 + 100), because there is a buffer size check at:
> dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/
> video_capture/video_capture_impl.cc?from=video_capture_impl.cc#l254
> 
> Is it a required check?  Or can we modify the CalcBufferSize() calculation
> in order to reserve more buffer?

Sorry for the delay in replying. I don't think we should modify CalcBufferSize(), since it is part of libyuv. However, we can modify the call in VP8EncoderImpl::InitEncode() to add a constant to the return value of CalcBufferSize().

Comment 8

[Shian-Yow Wu [:swu]]

Attachment: bug919979.patch

Comment 9

[Shian-Yow Wu [:swu]]

https://tbpl.mozilla.org/?tree=Try&rev=15b34860f5e5

Comment 10

[Shian-Yow Wu [:swu]]

Comment on attachment 8341524 [details] [diff] [review]
bug919979.patch

Tim,

Thanks for your comment.
The VP8 overhead for small resolution is 31 bytes, and the patch reserves 100 bytes for it.
Could you review it?

Comment 11

[Shian-Yow Wu [:swu]]

Previous try result showed some failures, this is the new one.
https://tbpl.mozilla.org/?tree=Try&rev=d2d99463b34d

Comment 12

[Timothy B. Terriberry (:derf)]

Comment on attachment 8341524 [details] [diff] [review]
bug919979.patch

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

::: media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc
@@ +170,5 @@
>    // allocate memory for encoded image
>    if (encoded_image_._buffer != NULL) {
>      delete [] encoded_image_._buffer;
>    }
> +  // Reserve more buffer for small resolution overhead.

It's not clear this comment applies to the magic number "100". Perhaps:
// Reserve 100 extra bytes for overhead at small resolutions.

Comment 13

[Shian-Yow Wu [:swu]]

Attachment: Patch: Reserve extra buffer for VP8 encoding. (r=derf)

Addressed review comments.

Comment 14

[Shian-Yow Wu [:swu]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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