1184616 - Uninitialised value use in android::OMXCodec::read

Status: RESOLVED INVALID

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

Description

[Julian Seward [:jseward]]

STR: Build Aries-KK (./config.sh aries) and run Valgrind as described at
https://developer.mozilla.org/en-US/Firefox_OS/Debugging/Debugging_B2G_using_valgrind

I think this occurs when running the browser and trying to play a 
Youtube video, but tbh I am not entirely sure.

Comment 1

[Julian Seward [:jseward]]

Attachment: Valgrind complaint

Comment 2

[Gregor Wagner [:gwagner]]

Anthony, can you help out?

Comment 3

[Julian Seward [:jseward]]

From a bit of digging around, it looks as if the problem is as follows:

frameworks/av/media/libstagefright/OMXCodec.cpp:4249 looks at
a BufferInfo::mOutputCropped field:

    if (info->mOutputCropChanged) {  <---- 4249
        initNativeWindowCrop();
        info->mOutputCropChanged = false;
    }

I believe the structure that |info| points at was created at
frameworks/av/media/libstagefright/OMXCodec.cpp:2050

        BufferInfo info;  <----- 2050
        info.mData = NULL;
        info.mSize = def.nBufferSize;
        info.mStatus = OWNED_BY_US;
        info.mMem = NULL;
        info.mMediaBuffer = new MediaBuffer(graphicBuffer);
        info.mMediaBuffer->setObserver(this);

This unfortunately fails to initialise mOutputCropChanged.

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Blake - do you want this one?

Comment 5

[Blake Wu [:bwu][:blakewu]]

Yeah. I can check it.

Comment 6

[Blake Wu [:bwu][:blakewu]]

Agree with comment 3, mOutputCropChanged is not initialized in allocateOutputBuffersFromNativeWindow(). 
It looks like it is a bug caused by CAF for the commit (9310a8d8ce6551046532f27989e1819b103e8d70) about "Smooth Streaming in OMXCodec". Android doesn't have mOutputCropChanged.

Comment 7

[Blake Wu [:bwu][:blakewu]]

Ni Carol to get CAF's attention.

Comment 8

[Greg Grisco]

Ok, I have a question out to our video experts to analyze this.  I hope to respond here tomorrow.

Comment 9

[Greg Grisco]

Yes, it's confirmed that this field needs to be initialized as stated above.  I'll create the patch and make sure it gets pushed to CAF.

Comment 10

[Blake Wu [:bwu][:blakewu]]

(In reply to Greg Grisco from comment #9)
> Yes, it's confirmed that this field needs to be initialized as stated above.
> I'll create the patch and make sure it gets pushed to CAF.
Thanks!