Android MediaCodec #3089

Merged
merged 2 commits into from Oct 3, 2013

Projects

None yet

6 participants

@davilla
Contributor
davilla commented Aug 10, 2013

Adds Android 4.1+ MediaCodec implementation for DVDPlayer.
This is POC, sw-rendering only and needs surface rendering added.
Works for 1080p/720p under Odroid/U2, 720p under Nexus7/Ouya (Tegra3).

Much thanks to Phaeodaria for surface rendering help.

@davilla
Contributor
davilla commented Aug 10, 2013

Yes, from a sw point of view COLOR_FormatYUV420Planar works best, three pointers to y,u, and v. Easy to diddle when you have large caches and plenty of registers. But from a hw point of view, it's terrible and the other flavors are preferred.

@theuni
Member
theuni commented Aug 10, 2013

I don't see the need for an eglimage, we're already easily backed by a gl texture. Android allows us to use this texure as decode target, which we then render with a custom shader. Grossly oversimplified procedure should be:

  • before configure, generate a gl texture and bind it to a surfacetexture via surface. stagefright does this. These bits should be abstracted so that this is threadsafe and avoids including gl headers in the codec. Non-abstracted looks like this:
GLint texture;
glGenTextures(1, &texture);
surfaceTexture = new CJNISurfaceTexture(texture);
surface = new CJNISurface(*surfaceTexture);
  • cache surface/surfaceTexture for passing to renderer
  • decode logic
  • render to texture:
releaseOutputBuffer(foo, true);

Then in renderer:

  • Texture is ready to be sync'd and rendered.
  • Renderer could implement SurfaceTexture.onFrameAvailable() to avoid communicating with the codec directly.
  • SurfaceTexture.updateTexImage() to sync with decode output. This binds the texture to GL_TEXTURE_EXTERNAL_OES target for us.
  • Bind custom fragment shader that uses samplerExternalOES shader rather than a normal 2d sampler.
  • Grab surface transform matrix
  • render as usual

@koying: Could you please explain why you took the extra steps of turning that texture into an eglimage and rendering it via an fbo?

@theuni
Member
theuni commented Aug 10, 2013

Ok, I think I see. Looks like stagefright is taking the backdoor to SurfaceTexture, and decoding into its backing store. MediaCodec has the advantage of decoding directly to the texture we specify, so that shouldn't be necessary.

@koying
Member
koying commented Aug 11, 2013

No, I also use the Surface texture plainly.

The problem I had was that using directly the texture means that "max decoder output buffers = max DVDVideoPicture = 1".
At the time, dvdplayer needed at least 2 DVDVideoPicture in the wild for flipping, so that was not possible. It might support this now, though.
Furthermore, that means you have to somehow block the decoding thread (be it my own in libstagefright or the internal one in MediaCodec) until we are done with the texture (for pts sync). It was cumbersome in my case, I don't even know about MediaCodec.

In your pseudo-code, there is no link between the frame that will come out as a result of updateTexImage and the pts we gave to dvdplayer, as the decoder might have decoded further frames up to our rendering.

So, basically, in my case, I had to duplicate the texture, to pass it to the renderer with proper pts handling and to avoid blocking the decoder thread. and I came up with fbo. I'm pretty sure there MUST be a better way than fbo, but I'm afraid texture duplication will have to be done anyway.

@davilla
Contributor
davilla commented Aug 20, 2013

forced pushed, not sure why ulions show up.

@davilla
Contributor
davilla commented Aug 20, 2013

There we go :)

@theuni
Member
theuni commented Aug 21, 2013

Please give theuni@52ca17d a shot.

Array elements are not garbage-collected if the global reference on the parent array is lost. They can be held individually, or you can hold a reference to the array which will also block the individual elements from gc.

The vector works fine for object arrays. The issue here was that array operations (push_back, insert, and friends) will cause an object copy, which means the destruction of our helper object and global ref. I added a helper to handle vector casting for us which solves this problem. The emplace_back() ensures that the object is not copied and our ref is not lost. It's possible that we need to add a copy ctor for some of the classes as well, but since I can't trigger the crash you're seeing, I didn't add that here.

If you can confirm the fix (I couldn't cause a crash before/after), I'll update the docs to reflect.

@davilla
Contributor
davilla commented Aug 21, 2013

This change works on both my test platforms and resolves the previous issues, please push it into xbmc.org mainline jni and I will sync and make the other bits here go away.

@davilla
Contributor
davilla commented Aug 23, 2013

updated.

@davilla
Contributor
davilla commented Sep 23, 2013

updated with prelim-surface rendering.

@davilla
Contributor
davilla commented Sep 23, 2013

jenkins build this please

@MartijnKaijser
Member

Windows got stuck hence failed build.

@koying
Member
koying commented Sep 23, 2013

Please split into logical commits. (just kidding :) )

As stated above, doing the "updateTexImage" inside the rendering loop means you are rendering from the android Surface frame queue, not from the xbmc frame queue.

This means that:

  • There is no direct link between the output of "CDVDVideoCodecAndroidMediaCodec::GetPicture" and what is rendered
  • You basically bypass dvdplayer post output frame generation
  • You have to hack the rendering loop ("// only render once") to try to keep both queues in sync.

Been there, tried that, rejected it. Are you really happy with that?

@davilla
Contributor
davilla commented Sep 23, 2013

1st pass, remember, this a POC/PR not a finished PR. There are several things that I'm not happy about. Those specific issues being some of them. But it sure is clean :)

@koying
Member
koying commented Sep 23, 2013

No prob, just highlighting.
Re clean, apply this to libstagefright and half the code is gone ;)

@davilla
Contributor
davilla commented Sep 23, 2013

ping @Phaeodaria :)

@Phaeodaria
Member

How many images are in flight at the same time. As long as there're no more than 2 frames in flight everything should work fine.

To ensure that the texture has been update after calling releaseOutputBuffer( index, true ) we could install an observer and wait for it with an mutex.

An alternative would be to call releaseOutputBuffer(index, true) in ::Render() and wait for the callback. The downside here is that the upload time is being added to Render().

With only two images in flight within the Renderer releaseOutputBuffer might also be called in the UploadTexImage function. I assume this one gets called one frame ahead so that the frame is ready for rendering at time.

@davilla
Contributor
davilla commented Sep 24, 2013

updated to move releaseOutputBuffer into renderer.

@davilla
Contributor
davilla commented Sep 26, 2013

now we are cooking with gas :)

@davilla
Contributor
davilla commented Sep 27, 2013

rebased to xbmc.org mainline and smashed.

@davilla
Contributor
davilla commented Sep 27, 2013

jenkins build this please

@koying
Member
koying commented Sep 27, 2013

Got a crash (with yesterday's ver): https://app.box.com/s/o2qejkn2hyc8qkilqmfv
It happened after a big forward.

@davilla
Contributor
davilla commented Sep 27, 2013

yea, seeks were an issue with yesterdays push. should be resolved now.

@Phaeodaria
Member

RENDER_FMT_TEXTURE_OES should be RENDER_FMT_MEDIACODEC

DVDVideoCodecAndroidMediaCodec.cpp:177
Is 'm_frameready->WaitMSec(20); ' still required since there's the mutex now?

DVDVideoCodecAndroidMediaCodec.cpp:808
boost::make_shared is the stanard way to create shared_ptrs:
m_surfaceTexture = boost::make_shared(new CJNISurfaceTexture(m_textureId));

Since you're using boost::shared_ptrs you could also use boost::scoped_ptr for CDVDVideoCodecAndroidMediaCodec::m_bitstream

@davilla
Contributor
davilla commented Sep 27, 2013

m_frameready->WaitMSec(20); means wait a max of 20ms unless the CEvent is fired, then return right away after it is fired. So if onFrameAvailable fires in 1ms, you return in 1ms.

@Phaeodaria
Member

In this case everyhing looks fine to me. Pull it in.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Sep 27, 2013
xbmc/android/jni/SurfaceTexture.cpp
+CJNISurfaceTextureOnFrameAvailableListener* CJNISurfaceTextureOnFrameAvailableListener::m_listenerInstance(NULL);
+
+CJNISurfaceTextureOnFrameAvailableListener::CJNISurfaceTextureOnFrameAvailableListener()
+: CJNIBase("org/xbmc/xbmc/XBMCOnFrameAvailableListener")
+{
+ CJNIContext *appInstance = CJNIContext::GetAppInstance();
+ if (!appInstance)
+ return;
+
+ // Convert "the/class/name" to "the.class.name" as loadClass() expects it.
+ std::string dotClassName = GetClassName();
+ for (std::string::iterator it = dotClassName.begin(); it != dotClassName.end(); ++it)
+ {
+ if (*it == '/')
+ *it = '.';
+ }
@jmarshallnz
jmarshallnz Sep 27, 2013 Member

trivial detail, but

#include <algorithm>
std::replace(dotClassName.begin(), dotClassName.end(), '/', '.');
@davilla
davilla Sep 28, 2013 Contributor

yep, already has been suggested

@davilla
davilla Sep 28, 2013 Contributor

xbmc / xbmc / android / jni / BroadcastReceiver.cpp needs that change too.

@jmarshallnz jmarshallnz and 2 others commented on an outdated diff Sep 27, 2013
system/shaders/guishader_frag_rgba_oes.glsl
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with XBMC; see the file COPYING. If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#extension GL_OES_EGL_image_external : require
+
+precision mediump float;
+uniform samplerExternalOES m_samp0;
+varying vec4 m_cord0;
+varying lowp vec4 m_colour;
+uniform int m_method;
@jmarshallnz
jmarshallnz Sep 27, 2013 Member

m_colour is unused, as is m_method. Dunno if this matters or not?

@davilla
davilla Sep 28, 2013 Contributor

my gles expert put it in, I assume he knows what he's doing.

@Phaeodaria
Phaeodaria Sep 28, 2013 Member

This is the equivalent to system/shaders/guishader_frag_rgba_oes.glsl with an oes sampler instead of an 2d texture. I was already wondering why this file had the m_colour. A clever glsl compiler will optimize it away in the linking step. A not so clever compiler will interpolate it which would be bad from a performance perspective.

If you're going to remove it please remove all unused varyings/uniforms, eventually also in the non oes version.

When querying a uniform which does not exists GLES will report -1 as uniform location. This is a valid input for all glUniform calls and thus no extra special 'does the uniform exist' checks are required on the C++ code side. In fact if the compiler decides that an uniform is not being used it'll also return -1.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Sep 27, 2013
xbmc/cores/VideoRenderers/LinuxRendererGLES.cpp
+
+ glUniformMatrix4fv(g_Windowing.GUIShaderGetCoord0Matrix(), 1, GL_FALSE, m_textureMatrix);
+
+ GLubyte idx[4] = {0, 1, 3, 2}; //determines order of triangle strip
+ GLfloat ver[4][4];
+ GLfloat tex[4][4];
+ float col[4][3];
+
+ for (int i = 0;i < 4;++i)
+ {
+ col[i][0] = col[i][1] = col[i][2] = 1.0;
+ }
+
+ GLint posLoc = g_Windowing.GUIShaderGetPos();
+ GLint texLoc = g_Windowing.GUIShaderGetCoord0();
+ GLint colLoc = g_Windowing.GUIShaderGetCol();
@jmarshallnz
jmarshallnz Sep 27, 2013 Member

As the colour is unused, you can possibly remove this and the glVertexAttribPointer/glEnableVertixAttribArray calls?

@Phaeodaria
Phaeodaria Sep 28, 2013 Member

Good point. Removing it a valid optimization and will not cause any trouble.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Sep 27, 2013
xbmc/cores/VideoRenderers/LinuxRendererGLES.h
@@ -207,6 +217,7 @@ class CLinuxRendererGLES : public CBaseRenderer
void RenderOpenMax(int index, int field); // OpenMAX rgb texture
void RenderEglImage(int index, int field); // Android OES texture
void RenderCoreVideoRef(int index, int field); // CoreVideo reference
+ void RenderMediaCodec(int index, int field); // CoreVideo reference
@jmarshallnz
jmarshallnz Sep 27, 2013 Member

inaccurate comment :)

@davilla
davilla Sep 28, 2013 Contributor

fixed.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Sep 27, 2013
...er/DVDCodecs/Video/DVDVideoCodecAndroidMediaCodec.cpp
+ }
+ }
+ if (m_codec)
+ break;
+ }
+ if (!m_codec)
+ {
+ CLog::Log(LOGERROR, "CDVDVideoCodecAndroidMediaCodec:: Failed to create Android MediaCodec");
+ SAFE_DELETE(m_bitstream);
+ return false;
+ }
+
+ m_opened = ConfigureMediaCodec();
+ if (!m_opened)
+ {
+ SAFE_DELETE(m_bitstream);
@jmarshallnz
jmarshallnz Sep 27, 2013 Member

Should m_codec be reset here, or doesn't matter? (Log?)

@davilla
davilla Sep 28, 2013 Contributor

actually does not matter as ConfigureMediaCodec always returns true, We will just make that little bit away and change the return of ConfigureMediaCodec to void.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Sep 27, 2013
...er/DVDCodecs/Video/DVDVideoCodecAndroidMediaCodec.cpp
+ m_videobuffer.mediacodec = NULL;
+
+ CLog::Log(LOGINFO, "CDVDVideoCodecAndroidMediaCodec:: "
+ "Open Android MediaCodec %s", m_codecname.c_str());
+
+ return true;
+}
+
+void CDVDVideoCodecAndroidMediaCodec::Dispose()
+{
+ m_opened = false;
+
+ // release any retained demux packets
+ while (!m_demux.empty())
+ {
+ amc_demux demux_pkt = m_demux.front();
@jmarshallnz
jmarshallnz Sep 27, 2013 Member

amc_demux &demux_pkt = m_demux.front();

@davilla
davilla Sep 28, 2013 Contributor

fixed.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Sep 27, 2013
...er/DVDCodecs/Video/DVDVideoCodecAndroidMediaCodec.cpp
+ free(demux_pkt.pData);
+ m_demux.pop();
+ }
+
+ // invalidate any inflight outputbuffers
+ // make sure m_output is empty so we do
+ // not create new ones.
+ m_input.clear();
+ m_output.clear();
+ FlushInternal();
+
+ // clear m_videobuffer bits
+ if (m_videobuffer.mediacodec)
+ m_videobuffer.mediacodec = NULL;
+ if (m_videobuffer.iFlags)
+ m_videobuffer.iFlags = 0;
@jmarshallnz
jmarshallnz Sep 27, 2013 Member

unless it's required for debugging, you don't need the if's here

@davilla
davilla Sep 28, 2013 Contributor

carryovers from prvious code, removed.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Sep 27, 2013
...er/DVDCodecs/Video/DVDVideoCodecAndroidMediaCodec.cpp
+ int index = m_codec->dequeueInputBuffer(timeout_us);
+ if (index >= 0)
+ {
+ // docs lie, getInputBuffers should be good after
+ // m_codec->start() but the internal refs are not
+ // setup until much later on some devices.
+ if (m_input.empty())
+ {
+ m_input = m_codec->getInputBuffers();
+ CLog::Log(LOGDEBUG, "CDVDVideoCodecAndroidMediaCodec::Decode, ibuffercount(%d)", m_input.size());
+ }
+
+ // we have an input buffer, fill it.
+ int size = m_input[index].capacity();
+ // fetch the front demux packet
+ demux_pkt = m_demux.front();
@jmarshallnz
jmarshallnz Sep 27, 2013 Member

You could use a reference here as well if you like, given that you go and pop it afterwards (not a big deal - only a tiny memcpy anyway).

@davilla
davilla Sep 28, 2013 Contributor

fixed.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Sep 27, 2013
...er/DVDCodecs/Video/DVDVideoCodecAndroidMediaCodec.cpp
+ }
+
+ return rtn;
+}
+
+void CDVDVideoCodecAndroidMediaCodec::Reset()
+{
+ CLog::Log(LOGDEBUG, "CDVDVideoCodecAndroidMediaCodec::Reset");
+
+ if (!m_opened)
+ return;
+
+ // dump any pending demux packets
+ while (!m_demux.empty())
+ {
+ amc_demux demux_pkt = m_demux.front();
@jmarshallnz
jmarshallnz Sep 27, 2013 Member

mayaswell use a reference

@davilla
davilla Sep 28, 2013 Contributor

fixed.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Sep 28, 2013
...er/DVDCodecs/Video/DVDVideoCodecAndroidMediaCodec.cpp
+ if (!m_inflight.empty())
+ {
+ for (size_t i = 0; i < m_inflight.size(); i++)
+ m_inflight[i]->Validate(false);
+ m_inflight.clear();
+ }
+
+ if (!m_output.empty())
+ {
+ for (size_t i = 0; i < m_output.size(); i++)
+ {
+ m_inflight.push_back(
+ new CDVDMediaCodecInfo(i, m_textureId, m_codec, m_surfaceTexture, m_frameAvailable)
+ );
+ }
+ }
@jmarshallnz
jmarshallnz Sep 28, 2013 Member

neither of the if()'s are required. Could use an iterator on the first (no big deal).

@davilla
davilla Sep 28, 2013 Contributor

fixed but I'll keep the array usage on the 1st.

@jmarshallnz jmarshallnz commented on the diff Sep 28, 2013
...ayer/DVDCodecs/Video/DVDVideoCodecAndroidMediaCodec.h
+ * any later version.
+ *
+ * This Program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with XBMC; see the file COPYING. If not, write to
+ * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ */
+
+#include <queue>
+#include <vector>
@jmarshallnz
jmarshallnz Sep 28, 2013 Member

Need a boost include here (likely it's being grabbed from somewhere else, but better to be explicit).

@davilla
davilla Sep 28, 2013 Contributor

fixed.

@jmarshallnz
Member

Nice and clean - great work.

@davilla
Contributor
davilla commented Sep 28, 2013

@koying any clue why standard 4.1 odroid barfs with this ?

E/ ( 2310): egl_android_pixel_format* egl_android_get_native_buffer_format(android_native_buffer_t) unsupported native buffer format (0x13)
E/ ( 2310): egl_image_ egl_create_image_ANDROID_native_buffer(egl_display, egl_context_, void_, EGLint_, void*) failed to map native buffer
E/SurfaceTexture( 2310): [unnamed-2310-0] error creating EGLImage: 0x3003
W/System.err( 2310): java.lang.RuntimeException: Error during updateTexImage (see logcat for details)
W/System.err( 2310): at android.graphics.SurfaceTexture.nativeUpdateTexImage(Native Method)
W/System.err( 2310): at android.graphics.SurfaceTexture.updateTexImage(SurfaceTexture.java:162)
W/System.err( 2310): at dalvik.system.NativeStart.run(Native Method)

@davilla
Contributor
davilla commented Sep 29, 2013

odroid issue is a android firmware opps. Someone did not pay attention to details and follow the call-chain, see http://forum.odroid.com/viewtopic.php?f=7&t=2380

@davilla
Contributor
davilla commented Oct 2, 2013

final squash, inject planned in 6 hours

@davilla davilla merged commit c15ebac into xbmc:master Oct 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment