Closed Bug 1157991 Opened 5 years ago Closed 5 years ago

aSample is overwritten if packet contains multiple samples in IntelWebMVideoDecoder::Demux

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: kinetik, Assigned: kinetik)

References

Details

Attachments

(1 file, 2 obsolete files)

If the loop at line 202 (https://hg.mozilla.org/mozilla-central/annotate/10a237997b8d/dom/media/webm/IntelWebMVideoDecoder.cpp#l202) runs more than one, subsequent VP8Sample allocations will overwrite earlier ones stored in the aSample out-parameter.

This was pointed out on IRC but nobody had bothered to file a bug for it.
Hi Matthew, I looked at this code and it appears we have a memory leak. I think the best option is to simply hoist the "new VP8Sample" call on line 202 outside the loop and function and instantiate only one VP8Sample object per instance of the class. Then line 202 will just be a reassignment on successive calls. If you concur I'll make the change and submit a patch.
We can remove the loop entirely and return an error if count > 1.  As a historical accident (I think?) we ended up with code handling multiple frames per packet in the WebM code.  In practice this never happens, so we're better off to remove the handling and have simpler code.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Attached patch bug1157991_v0.patch (obsolete) — Splinter Review
Attachment #8611622 - Flags: review?(giles)
Duplicate of this bug: 1159593
Comment on attachment 8611622 [details] [diff] [review]
bug1157991_v0.patch

Forgot to update the comment in DemuxPacket().
Attachment #8611622 - Attachment is obsolete: true
Attachment #8611622 - Flags: review?(giles)
Attached patch bug1157991_v1.patch (obsolete) — Splinter Review
Attachment #8612074 - Flags: review?(giles)
And for ease of later code archaeology: the bug I duped to this one points to bug 1158226 comment 10 as the justification for this change.
Helps if I attach the right file.  Apologies for bug/review spam.
Attachment #8612074 - Attachment is obsolete: true
Attachment #8612074 - Flags: review?(giles)
Attachment #8612114 - Flags: review?(giles)
Comment on attachment 8612114 [details] [diff] [review]
bug1157991_v1.patch

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

Sorry for the review delay. Missed this yesterday.

I guess we were already assuming the one-frame-per-packet anyway. Do we have a test for the rejecting files which aren't?

::: dom/media/webm/WebMReader.cpp
@@ +929,5 @@
>      if (r == -1) {
>        return nullptr;
>      }
> +    vpx_codec_stream_info_t si;
> +    memset(&si, 0, sizeof(si));

BTW mozilla style would have this as PodZero(&si); these days. Better to be consistent with the rest of the file though.
Attachment #8612114 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) from comment #9)
> I guess we were already assuming the one-frame-per-packet anyway. Do we have
> a test for the rejecting files which aren't?

We don't, but I'll generate one and add it.
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/74e541ff80e1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.