Closed Bug 1026854 Opened 6 years ago Closed 6 years ago

We need RAII for memory management in media/libcubeb/src/cubeb_resampler.cpp

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jwwang, Assigned: cgg, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 5 obsolete files)

See bug 1008079 comment 24. This should be a good bug for the Bugzilla starter.
Mentor: jwwang
Depends on: 1008079
Whiteboard: [good first bug]
Attached patch WIP (obsolete) — Splinter Review
:-*
Attachment #8443941 - Attachment is obsolete: true
Attachment #8443956 - Flags: review?(paul)
:3
Attachment #8443956 - Attachment is obsolete: true
Attachment #8443956 - Flags: review?(paul)
Attachment #8443958 - Flags: review?(paul)
Voila, on y est.
Attachment #8443960 - Flags: review?(paul)
Attachment #8443958 - Attachment is obsolete: true
Attachment #8443958 - Flags: review?(paul)
Comment on attachment 8443960 [details] [diff] [review]
Add an utilitary class for array RAII

Looks good.
Attachment #8443960 - Flags: review?(paul) → review+
Assignee: nobody → clement.geiger
> , leftover_frames_buffer(auto_array<uint8_t>(frames_to_bytes(params, leftover_frame_size)))
This only works because all compilers involved happen to do copy elision: http://en.wikipedia.org/wiki/Copy_elision

Maybe remove the "auto_array<uint8_t>(" and declare a private and undefined copy-constructor and assignment operator to avoid the footgun?
Indeed you're completely right. I'll do that now. Thank you!
Comment on attachment 8443960 [details] [diff] [review]
Add an utilitary class for array RAII

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

::: media/libcubeb/src/cubeb_resampler.cpp
@@ +17,5 @@
> +template<typename T>
> +class auto_array
> +{
> +public:
> +  auto_array(uint32_t size)

Add 'explicit' to avoid confusing implicit conversion.

@@ +32,5 @@
> +    return data;
> +  }
> +
> +private:
> +  T * data;

|T * const data| for const-correctness.

@@ +127,4 @@
>    // A little buffer to store the leftover frames,
>    // that is, the samples not consumed by the resampler that we will end up
>    // using next time fill() is called.
> +  auto_array<uint8_t> leftover_frames_buffer;

const auto_array<> ...

@@ +131,2 @@
>    // A buffer to store frames that will be consumed by the resampler.
> +  auto_array<uint8_t> resampling_src_buffer;

const auto_array<> ...
We also need one for |SpeexResamplerState *|.
https://hg.mozilla.org/mozilla-central/rev/619b13c76a5f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
It would be a plus to have boundary check for the array class.
Hi JW, thanks for the feedback. I agree about the const. About the boundary check, I dit it at first and then realized that the [] operator was never used... so I took it out.
I think it would be nice to add some mutator functions to replace memcpy at [1] for safe array operation since we can check array boundary in those mutators.

[1] http://hg.mozilla.org/mozilla-central/file/f78e532e8a10/media/libcubeb/src/cubeb_resampler.cpp#l157
Attached patch 1026854 (obsolete) — Splinter Review
Attachment #8444071 - Attachment is obsolete: true
Attachment #8444071 - Flags: review?(paul)
Attachment #8447493 - Flags: review?(paul)
Comment on attachment 8447493 [details] [diff] [review]
1026854

Non.
Attachment #8447493 - Attachment is obsolete: true
Attachment #8447493 - Flags: review?(paul)
You need to log in before you can comment on or make changes to this bug.