Closed Bug 1098994 Opened 5 years ago Closed 5 years ago

Update output buffer when get INFO_OUTPUT_BUFFERS_CHANGED event from omx

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: ayang, Assigned: ayang)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch update_output_buffer (obsolete) — Splinter Review
Attachment #8522803 - Flags: feedback?(bwu)
Attached patch update_output_buffer (obsolete) — Splinter Review
Update nit.
Attachment #8522803 - Attachment is obsolete: true
Attachment #8522803 - Flags: feedback?(bwu)
Attachment #8522805 - Flags: feedback?(bwu)
Btw, this video still can't be displayed after checkin this patch. This bug fix the output buffer crashed problem in color convertion.
This video's timescale is "1000000", not 90000. It looks like demuxer doesn't handle it well. I'll fire another bug to check demuxer problem.

Following is MP4Box's ISO dump info of this video:

MediaHeaderBox CreationTime="3475736040" ModificationTime="3475736040" TimeScale="1000000" Duration="28440000" LanguageCode="und"
Comment on attachment 8522805 [details] [diff] [review]
update_output_buffer

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

Looks good to me!

::: dom/media/omx/MediaCodecProxy.cpp
@@ +502,5 @@
>    return true;
>  }
>  
> +bool MediaCodecProxy::UpdateOutputBuffer()
> +{

should we have the check if (mCodec == nullptr)?
Attachment #8522805 - Flags: feedback?(bwu) → feedback+
Attached patch update_output_buffer (obsolete) — Splinter Review
Updated according to feedback comment.
Attachment #8522805 - Attachment is obsolete: true
Attachment #8522857 - Flags: review?(edwin)
Comment on attachment 8522857 [details] [diff] [review]
update_output_buffer

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

::: dom/media/omx/MediaCodecProxy.h
@@ +133,5 @@
>    bool Prepare();
>    bool IsWaitingResources();
>    bool IsDormantNeeded();
>    void ReleaseMediaResources();
> +  // It updates mOutputBuffer when receiving INFO_OUTPUT_BUFFERS_CHANGED event.

nit: "This updates ..."

@@ +134,5 @@
>    bool IsWaitingResources();
>    bool IsDormantNeeded();
>    void ReleaseMediaResources();
> +  // It updates mOutputBuffer when receiving INFO_OUTPUT_BUFFERS_CHANGED event.
> +  bool UpdateOutputBuffer();

nit: Should be UpdateOutputBuffers()
Attachment #8522857 - Flags: review?(edwin) → review+
(In reply to Edwin Flores [:eflores] [:edwin] from comment #6)
> Comment on attachment 8522857 [details] [diff] [review]
> update_output_buffer
> 
> Review of attachment 8522857 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/omx/MediaCodecProxy.h
> @@ +133,5 @@
> >    bool Prepare();
> >    bool IsWaitingResources();
> >    bool IsDormantNeeded();
> >    void ReleaseMediaResources();
> > +  // It updates mOutputBuffer when receiving INFO_OUTPUT_BUFFERS_CHANGED event.
> 
> nit: "This updates ..."
> 
> @@ +134,5 @@
> >    bool IsWaitingResources();
> >    bool IsDormantNeeded();
> >    void ReleaseMediaResources();
> > +  // It updates mOutputBuffer when receiving INFO_OUTPUT_BUFFERS_CHANGED event.
> > +  bool UpdateOutputBuffer();
> 
> nit: Should be UpdateOutputBuffers()

Thanks for review.
I'll add another patch.
Attachment #8522857 - Attachment is obsolete: true
Attachment #8523615 - Flags: review+
It won't break try because there is no testcase for this so far.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aa52973f5a7d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.