Closed Bug 1128410 Opened 5 years ago Closed 5 years ago

Don't error on 8K videos and dynamically manage libstagefright memory.

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 + fixed
firefox37 + fixed
firefox38 + fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

The MP4Demuxer attempts to allocate a buffer and if that buffer is more than 3MB (size of a YUV 1080p frame), will fail.

It's a silly restriction.

It prevents playing any videos > 1920x1080
Should add that we don't even need that buffer with fragmented MP4.
(In reply to Jean-Yves Avenard [:jya] from comment #0)
> The MP4Demuxer attempts to allocate a buffer and if that buffer is more than
> 3MB (size of a YUV 1080p frame), will fail.
MP4Demuxer should only handle encoded data, instead of YUV data, right?
The theory is that the maximum size of a NAL is of an uncompressed video frame (which are in YUV format)
Allocate the MP$ parser buffer lazily. Allows > 1080p playback in youtube
Attachment #8557761 - Flags: review?(ajones)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Attachment #8557761 - Flags: review?(ajones) → review+
use fallible dynamic memory allocation
Attachment #8557806 - Flags: review?(ajones)
Attachment #8557806 - Flags: review?(ajones) → review+
oops... forgot to remove the deallocation of mSrcBuffer
Attachment #8557806 - Attachment is obsolete: true
Summary: Can't play videos > 1080p → Don't error on 8K videos and dynamically manage libstagefright memory.
Comment on attachment 8557761 [details] [diff] [review]
Lazily allocate the MP4 parser buffer

Approval Request Comment
[Feature/regressing bug #]:1127173
[User impact if declined]:OOM/Crashes
[Describe test coverage new/current, TreeHerder]:Just pushed to m-i
[Risks and why]: Risks are considered low. This is a code that isn't actually in use with MSE but memory was allocated regardless. For non-mse, memory allocations are now fallible.
[String/UUID change made/needed]:none
Attachment #8557761 - Flags: approval-mozilla-beta?
Attachment #8557761 - Flags: approval-mozilla-aurora?
Comment on attachment 8557811 [details] [diff] [review]
Part2. Make memory allocation dynamic

Approval Request Comment
[Feature/regressing bug #]:1127173
[User impact if declined]:OOM/Crashes
[Describe test coverage new/current, TreeHerder]:Just pushed to m-i
[Risks and why]: Risks are considered low. This is a code that isn't actually in use with MSE but memory was allocated regardless. For non-mse, memory allocations are now fallible.
[String/UUID change made/needed]:none
Attachment #8557811 - Flags: approval-mozilla-beta?
Attachment #8557811 - Flags: approval-mozilla-aurora?
Tracking 36+ as this bug is said to fix bug 1127173, which is an OOM crash.
I'm good to take this change on Aurora but want to see it hit m-c first. Sylvestre will make the call for Beta as he's tracking all MSE related work in 36.
Attachment #8557761 - Flags: approval-mozilla-beta?
Attachment #8557761 - Flags: approval-mozilla-beta+
Attachment #8557761 - Flags: approval-mozilla-aurora?
Attachment #8557761 - Flags: approval-mozilla-aurora+
Comment on attachment 8557811 [details] [diff] [review]
Part2. Make memory allocation dynamic

Accepting now because we have too much OOM in 36 with video... and we want to take as much video related patch asap.
Attachment #8557811 - Flags: approval-mozilla-beta?
Attachment #8557811 - Flags: approval-mozilla-beta+
Attachment #8557811 - Flags: approval-mozilla-aurora?
Attachment #8557811 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/a528c8fe948e
https://hg.mozilla.org/mozilla-central/rev/68581d6d62cd
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.