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

RESOLVED FIXED in mozilla28

Status

()

Core
WebRTC: Audio/Video
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: swu, Assigned: swu)

Tracking

unspecified
mozilla28
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 809115 [details]
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.
tim - is this fixed in a newer release?
Assignee: nobody → rjesup
Flags: needinfo?(tterribe)

Updated

5 years ago
Assignee: rjesup → nobody
(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.
Flags: needinfo?(tterribe)
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
Sorry, to clarify again.
The resolution been tested was 2x2 with frame legnth 6 allocated in the above example.
(Assignee)

Comment 6

5 years ago
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?
Flags: needinfo?(tterribe)
(Assignee)

Updated

5 years ago
Summary: Potential crash after WebrtcVideoConduit::SelectSendResolution() called → Crash when calling WebrtcVideoConduit::SendVideoFrame() with very small resolution
(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().
Flags: needinfo?(tterribe)
(Assignee)

Comment 8

4 years ago
Created attachment 8341524 [details] [diff] [review]
bug919979.patch
Assignee: nobody → swu
Status: NEW → ASSIGNED
(Assignee)

Comment 10

4 years ago
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?
Attachment #8341524 - Flags: review?(tterribe)
(Assignee)

Comment 11

4 years ago
Previous try result showed some failures, this is the new one.
https://tbpl.mozilla.org/?tree=Try&rev=d2d99463b34d
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.
Attachment #8341524 - Flags: review?(tterribe) → review+
(Assignee)

Comment 13

4 years ago
Created attachment 8342093 [details] [diff] [review]
Patch: Reserve extra buffer for VP8 encoding.  (r=derf)

Addressed review comments.
Attachment #8341524 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ee8b8fa121d0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.