941274 - Fix ResetDecode() in gonk to be consistent to other platofroms

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Sotaro Ikeda [:sotaro]]

In gonk, ResetDecode() is used to flush video and audio buffers in media framework and also graphics pipe line. But this api expect to flush only flush video audio buffers in media framework. And  MediaOmxReader::Seek() does flush video and audio buffers in media framework without using ResetDecode().

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix ResetDecode in MediaOmxReader

Comment 2

[Sotaro Ikeda [:sotaro]]

Comment 0 is agreement in media playback work week in Auckland.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8340110 [details] [diff] [review]
patch - Fix ResetDecode in MediaOmxReader

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

::: content/media/omx/MediaOmxReader.cpp
@@ +176,5 @@
>    return NS_OK;
>  }
>  
>  // Resets all state related to decoding, emptying all buffers etc.
>  nsresult MediaOmxReader::ResetDecode()

Do you have to do anything here to flush partially decoded frames in libstagefright's decoder? Otherwise p-frames after the seek may reference the wrong frame?

Should you call container->GetImageContainer()->ClearAllImagesExceptFront() here instead of in Seek()?

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Chris Pearce (:cpearce) from comment #3)
> Comment on attachment 8340110 [details] [diff] [review]
> patch - Fix ResetDecode in MediaOmxReader
> 
> Review of attachment 8340110 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/MediaOmxReader.cpp
> @@ +176,5 @@
> >    return NS_OK;
> >  }
> >  
> >  // Resets all state related to decoding, emptying all buffers etc.
> >  nsresult MediaOmxReader::ResetDecode()
> 
> Do you have to do anything here to flush partially decoded frames in
> libstagefright's decoder? Otherwise p-frames after the seek may reference
> the wrong frame?

The partially decoded frame in the decoder is automatically freed by seek read like the following.

>    MediaSource::ReadOptions options;
>    options.setSeekTo(aTimeUs, MediaSource::ReadOptions::SEEK_PREVIOUS_SYNC);
>    err = mVideoSource->read(&mVideoBuffer, &options);

http://mxr.mozilla.org/mozilla-central/source/content/media/omx/OmxDecoder.cpp#743

> Should you call container->GetImageContainer()->ClearAllImagesExceptFront()
> here instead of in Seek()?

Do not need to call ClearAllImagesExceptFront() in ResetDecode(). ResetDecode() does not know how much flush graphic pipe line. Seek() need to flush all video frames except front. But before destroy the video codec, need to flush all video frames.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

In that case, you don't need to override MediaOmxReader::ResetDecode(). MediaOmxReader::ResetDecode() just calls MediaDecoderReader::ResetDecode(), but that will happen anyway if you remove MediaOmxReader::ResetDecode().

Comment 6

[Sotaro Ikeda [:sotaro]]

Yeah, I will update the patch.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver 2 - Fix ResetDecode in MediaOmxReader

Remove unnecessary MediaOmxReader::ResetDecode().

Comment 8

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=1468d1d9b66f

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver 3 - Fix ResetDecode in MediaOmxReader

Committable patch. Carry "doublec review+".

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/806ed2a530f9

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/806ed2a530f9