1127654 - Remove monitor from gonk PlatformDeocdeModule

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alfredo Yang (:alfredo)]

Follow up bug of from https://bugzilla.mozilla.org/show_bug.cgi?id=1122447#c15.

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: remove_monitor

Comment 2

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

Comment on attachment 8562560 [details] [diff] [review]
remove_monitor

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

LGTM. Have some nits and questions for you.

::: dom/media/fmp4/gonk/GonkAudioDecoderManager.h
@@ +22,5 @@
>  
>  class GonkAudioDecoderManager : public GonkDecoderManager {
>  typedef android::MediaCodecProxy MediaCodecProxy;
>  public:
> +  GonkAudioDecoderManager(MediaTaskQueue* aTaskQueue, const mp4_demuxer::AudioDecoderConfig& aConfig);

nit: alignment

::: dom/media/fmp4/gonk/GonkDecoderModule.cpp
@@ +40,5 @@
>                                       MediaDataDecoderCallback* aCallback)
>  {
>    nsRefPtr<MediaDataDecoder> decoder =
> +  new GonkMediaDataDecoder(new GonkVideoDecoderManager(aVideoTaskQueue,
> +                                                       aImageContainer,aConfig),

nit: Space after ","

::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
@@ +412,5 @@
>          GVDM_LOG("Failed to create video data");
>          return NS_ERROR_UNEXPECTED;
>        }
>        aOutData = data;
> +      mDecoder->flush();

Is it required for EOS since it should be used for seek case?

@@ +487,5 @@
> +    }
> +
> +    GonkVideoDecoderManager* mManager;
> +  };
> +

This class is similar to the one GonkDecoderManager's Flush(). Could this to be put in a header file and be used by GonkDecoderManager as well?

::: dom/media/fmp4/gonk/GonkVideoDecoderManager.h
@@ +37,5 @@
>  typedef android::MediaCodecProxy MediaCodecProxy;
>  typedef mozilla::layers::TextureClient TextureClient;
>  
>  public:
> +  GonkVideoDecoderManager(MediaTaskQueue* aTaskQueue, mozilla::layers::ImageContainer* aImageContainer,

nit: alignment

Comment 3

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

Comment on attachment 8562560 [details] [diff] [review]
remove_monitor

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

::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
@@ +487,5 @@
> +    }
> +
> +    GonkVideoDecoderManager* mManager;
> +  };
> +

Sorry. I didn't notice these two class name is different.

Comment 4

[Alfredo Yang (:alfredo)]

Comment on attachment 8562560 [details] [diff] [review]
remove_monitor

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

::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
@@ +412,5 @@
>          GVDM_LOG("Failed to create video data");
>          return NS_ERROR_UNEXPECTED;
>        }
>        aOutData = data;
> +      mDecoder->flush();

According to MediaCodec document:

"The codec will continue to return output buffers until it eventually signals the end of the output stream by specifying the same flag (BUFFER_FLAG_END_OF_STREAM) on the BufferInfo returned in dequeueOutputBuffer(MediaCodec.BufferInfo, long). Do not submit additional input buffers after signaling the end of the input stream, unless the codec has been flushed, or stopped and restarted."

It needs to send a flush after EOS if the player seeks to another place after EOS. Maybe it should be better to fix at bug 1131467, I'll remove this.

@@ +487,5 @@
> +    }
> +
> +    GonkVideoDecoderManager* mManager;
> +  };
> +

The Run() method in both classes are different, it requirements more complicated logic to put them into one class.

Comment 5

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

(In reply to Alfredo Yang from comment #4)
> Comment on attachment 8562560 [details] [diff] [review]
> remove_monitor
> 
> Review of attachment 8562560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
> @@ +412,5 @@
> >          GVDM_LOG("Failed to create video data");
> >          return NS_ERROR_UNEXPECTED;
> >        }
> >        aOutData = data;
> > +      mDecoder->flush();
> 
> According to MediaCodec document:
> 
> "The codec will continue to return output buffers until it eventually
> signals the end of the output stream by specifying the same flag
> (BUFFER_FLAG_END_OF_STREAM) on the BufferInfo returned in
> dequeueOutputBuffer(MediaCodec.BufferInfo, long). Do not submit additional
> input buffers after signaling the end of the input stream, unless the codec
> has been flushed, or stopped and restarted."
> 
> It needs to send a flush after EOS if the player seeks to another place
> after EOS. Maybe it should be better to fix at bug 1131467, I'll remove this.
Yeah. flush() would be better to be put in the seek case. 
> 
> @@ +487,5 @@
> > +    }
> > +
> > +    GonkVideoDecoderManager* mManager;
> > +  };
> > +
> 
> The Run() method in both classes are different, it requirements more
> complicated logic to put them into one class.
Understood.

Comment 6

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

Comment on attachment 8562560 [details] [diff] [review]
remove_monitor

r+ with comment 5.

Comment 7

[Alfredo Yang (:alfredo)]

Attachment: remove_monitor

Comment 8

[Carsten Book [:Tomcat]]

can we get a try run here ? Thanks!

Comment 9

[Alfredo Yang (:alfredo)]

(In reply to Carsten Book [:Tomcat] from comment #8)
> can we get a try run here ? Thanks!

Sure.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=862ca4f5b1db

Waiting the result.

Comment 10

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

(In reply to Alfredo Yang from comment #9)
> (In reply to Carsten Book [:Tomcat] from comment #8)
> > can we get a try run here ? Thanks!
> 
> Sure.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=862ca4f5b1db
> 
> Waiting the result.
The results look good.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a09191214534

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a09191214534