1340124 - RemoteDataDecoder may write on a too small surface

Status: RESOLVED INVALID

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

Description

[Jean-Yves Avenard [:jya]]

Per bug 1336431 comment 4.

Following bug 1299105; it is possible for the RemoteDataDecoder to start writing on a surface that is too small.

This would cause out of bound memory, and could be a safety concern.

In bug 1336431. The mechanism was put in place, that would allow to easily resize the surface allocated, or tear-down the MediaCodec decoder and create a new one.

Needless to say, this is the most serious problem introduced in bug 1299105, and it should be backed out.

Comment 1

[Jean-Yves Avenard [:jya]]

[Tracking Requested - why for this release]: There's potential for out of bound memory access.

Comment 2

[John Lin [:jhlin][:jolin]]

If I understand correctly, the concern is that decoder would allocates buffers for some (small) resolution and then later receives requests to decode frames larger than allocated buffers, right?

Based on my impression of the 'adaptive playback' functionality in Android [1], the patch in bug 1299105 should be fine for videos <= Full HD. And I believe that decoder will report error if frames exceed nMaxFrameWidth x nMaxFrameHeight. However, decoder on new released devices, such as Google Pixel, does support up to 4K/4096x2160 and could be problematic.

How about replacing the hard-coded values with queried ones and rejecting inputs larger than max resolution in ConfigurationChanged()? We should also add tests to make that resolution change will not cause the problem you raised.

[1] https://android.googlesource.com/platform/frameworks/native/+/897fe45cf7addc975fc7f9751c88ac53d9b9a778/include/media/hardware/HardwareAPI.h#101

Comment 3

[Jean-Yves Avenard [:jya]]

The RemoteDataDecoder is now drained then flushed prior a change of resolution.

By the time a new frame is fed, RemoteDataDecoder::ConfigurationChanged has been called, there's no buffered frames left anywhere.

So to me, the best approach is to simply reallocate a new buffer with the proper size. Always allocating the maximum size allowed seems like a waste of space to me. And forbidding resolutions higher than what we originally allocated a downside that we can easily do without.

Comment 4

[John Lin [:jhlin][:jolin]]

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> The RemoteDataDecoder is now drained then flushed prior a change of
> resolution.
> 
> By the time a new frame is fed, RemoteDataDecoder::ConfigurationChanged has
> been called, there's no buffered frames left anywhere.

 Yeah, just realized that I overlooked the P2 patch. :$

Comment 5

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

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> The RemoteDataDecoder is now drained then flushed prior a change of
> resolution.
It seems still takes noticeable time in handling resolution change. I have tried to set media.decoder.recycle.enabled false with the latest nightly on my Pixel XL and still can see video[1] playback stutter. It should be no surprises that most devices which are not powerful as Pixel XL may have much bad performance. 
> 
> By the time a new frame is fed, RemoteDataDecoder::ConfigurationChanged has
> been called, there's no buffered frames left anywhere.
> 
> So to me, the best approach is to simply reallocate a new buffer with the
> proper size. Always allocating the maximum size allowed seems like a waste
I think it is a trade-off between memory waste and seamless playback. Android's 'adaptive playback'[2] needs to allocate big memory for dynamical resolution change. Since Android's adaptive playback[3] has been supported from Android 4.4, it should be quite stable. It would surprise me if it could cause out of bound memory. Anyway, for the case that the size of incoming frame is larger the max size set in bug 1299105, we should follow the normal way (drain and then flush) and John has some idea. :-)      


[1]http://s3.cf.web.us.aiv-cdn.net/HTML5PartnerTest.html?asin=B00TYBBNAW&videoType=Trailer&ui
[2]https://android.googlesource.com/platform/frameworks/native/+/897fe45cf7addc975fc7f9751c88ac53d9b9a778/include/media/hardware/HardwareAPI.h#95
[3]https://developer.android.com/reference/android/media/MediaCodecInfo.CodecCapabilities.html#FEATURE_AdaptivePlayback

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #5)
> from Android 4.4, it should be quite stable. It would surprise me if it
> could cause out of bound memory. Anyway, for the case that the size of
> incoming frame is larger the max size set in bug 1299105, we should follow
> the normal way (drain and then flush) and John has some idea. :-)      

the out of bound memory is something we do, as we give a surface allocated that may not be big enough...

it has nothing to do with android 4.4 API being stable enough

Comment 7

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

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #5)
> > from Android 4.4, it should be quite stable. It would surprise me if it
> > could cause out of bound memory. Anyway, for the case that the size of
> > incoming frame is larger the max size set in bug 1299105, we should follow
> > the normal way (drain and then flush) and John has some idea. :-)      
> 
> the out of bound memory is something we do, as we give a surface allocated
> that may not be big enough...
> 
> it has nothing to do with android 4.4 API being stable enough
Very likely, I could be wrong.:-) This is the core of this bug that we need to make sure. 
 
John, 
Can you help check it further? We need to be certain about it. 
Thanks.

Comment 8

[Julien Cristau [:jcristau]]

Tracking for 52 to make sure we get to a resolution before 52.0b9, either here or in bug 1340480.

Comment 9

[John Lin [:jhlin][:jolin]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #7)
> (In reply to Jean-Yves Avenard [:jya] from comment #6)
> > the out of bound memory is something we do, as we give a surface allocated
> > that may not be big enough...
> > 
> > it has nothing to do with android 4.4 API being stable enough
> Very likely, I could be wrong.:-) This is the core of this bug that we need
> to make sure. 
>  
> John, 
> Can you help check it further? We need to be certain about it. 
> Thanks.

 Bug 1299105 sets max width/height to be 1920x1080, so that will be the size of output buffers. It's unclear what would happen when frames are larger than that.

Comment 10

[John Lin [:jhlin][:jolin]]

http://players.akamai.com/dash has a test MP4 stream with resolutions from 640x480 to 3840x2160 and it shows the resolution in frame images. I've tested it with Pixel, LG G5, Sony Z3C, and Nexus 5. All start with 768x432 or less and then play up to 3840x2160. I think we should run it on more devices to see how video > FHD will be handled.

Comment 11

[Ryan VanderMeulen (PTO, back July 27)]

Fixed on 52 by the backout in bug 1340480.
https://hg.mozilla.org/releases/mozilla-beta/rev/b70b7650fa1d

Comment 12

[John Lin [:jhlin][:jolin]]

As mentioned in bug 1299105 comment 82, decoder (producer) supporting adaptive playback should allocate enough size for video that chanages resolution and consumer (GLConsumer [1] in this case) that uses the buffers will examine the buffer info to adapt. 

Unless adaptive playback implementation is buggy, I think it should be safe to use the feature without worrying about buffer overrun issue.

[1] http://androidxref.com/4.4.4_r1/xref/frameworks/native/libs/gui/GLConsumer.cpp

Comment 13

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

Echo to comment 12, 
Basically, we use Android's adaptive playback feature to achieve seamless playback. For the buffers in the surface, we do give an initial size but allocation and resize are all handled in Android framework and decoder driver, not in our gecko. We don't touch it and actually we cannot. As I mentioned in comment 5, it has been supported since from Android 4.4, chromium[1] on Android also uses it, like many Android Apps do. So far, I don't see any blockers to prevent us from using it. Therefore, I am going to close this bug as invalid. 

[1]https://cs.chromium.org/chromium/src/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java?type=cs&q=supportsAdaptivePlayback+package:%5Echromium$&l=515

Comment 14

[Liz Henry (:lizzard)]

Though the backout in bug 13404080 was only on 52, it sounds like this isn't considered a problem on 53/54/55 now.