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

RESOLVED FIXED in mozilla28

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

Trunk
mozilla28
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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().
(Assignee)

Updated

5 years ago
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Comment 1

5 years ago
Created attachment 8340110 [details] [diff] [review]
patch - Fix ResetDecode in MediaOmxReader
(Assignee)

Comment 2

5 years ago
Comment 0 is agreement in media playback work week in Auckland.
(Assignee)

Updated

5 years ago
Version: 26 Branch → Trunk
(Assignee)

Updated

5 years ago
Attachment #8340110 - Flags: review?(chris.double)
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()?
(Assignee)

Comment 4

5 years ago
(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.
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().
(Assignee)

Comment 6

5 years ago
Yeah, I will update the patch.
(Assignee)

Comment 7

5 years ago
Created attachment 8340134 [details] [diff] [review]
patch ver 2 - Fix ResetDecode in MediaOmxReader

Remove unnecessary MediaOmxReader::ResetDecode().
(Assignee)

Updated

5 years ago
Attachment #8340110 - Attachment is obsolete: true
Attachment #8340110 - Flags: review?(chris.double)
(Assignee)

Updated

5 years ago
Attachment #8340134 - Flags: review?(chris.double)

Updated

5 years ago
Attachment #8340134 - Flags: review?(chris.double) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 8341441 [details] [diff] [review]
patch ver 3 - Fix ResetDecode in MediaOmxReader

Committable patch. Carry "doublec review+".
Attachment #8340134 - Attachment is obsolete: true
Attachment #8341441 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/806ed2a530f9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.