Closed Bug 1314863 Opened 3 years ago Closed 3 years ago

Data sample fed potentially fed twice to the decoder

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

(Keywords: regression)

Attachments

(1 file)

If we recreate decoder and init the decoder in [1], we should make sure the decoder have been initialized(promise resolved) then feed the sample to decoder input.

It may be a defect in current code implementation[2], could have an elegant way to deal with this.




[1]
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/media/platforms/wrappers/H264Converter.cpp#281

[2]
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/media/platforms/wrappers/H264Converter.cpp#113
I would like to work on this as a beginner of media playback rookie.
Assignee: nobody → jacheng
By adding some logs,

I found that when we met below scenario,

[1] Queue the incoming sample and wait for decoder initialized
http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/dom/media/platforms/wrappers/H264Converter.cpp#225

[2] Input sample which may be converted into Annex B format to an initializing not finished decoder.
http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/dom/media/platforms/wrappers/H264Converter.cpp#113

[3] Input the same sample[1](but not converted to Annex B) twice to the initialized decoder.
http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/dom/media/platforms/wrappers/H264Converter.cpp#248

Is the scenario dealing with the input sample correctly(input twice for the AnnexB and non-AnnexB sample is OK?)?

If it is OK, I think it makes me feel confused and hard to realize.

So I modified the code as the attached patch.

Hi jya, could you please guide me if I misunderstand anything or it is really a potential bug?

Thank you.
Attachment #8809271 - Flags: review?(jyavenard)
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91822

You now have two different places where you initialise the decoder and report and handle an error. that's duplicated code.

But I think you took a too complicated approach.

Let's look at the problem at hand:
- The decoder will be fed a sample before the initialisation has completed.

So what we want to do, is not feed the decoder until initalisation has completed right?

We already have a mechanism in place for a value returned by CreateDecoderAndInit to indicate we need to postpone any further action: NS_ERROR_NOT_INITIALIZED.
CheckForSPSChange returns the value returned by CreateDecoderAndInit.

As such, there's only two things to do:
1- Have CreateDecoderAndInit return NS_ERROR_NOT_INITIALIZED after calling mDecoder->Init().
2- Handle the returned value if its NS_ERROR_NOT_INITIALIZED like the earlier line:

    if (rv == NS_ERROR_NOT_INITIALIZED) {
      // Decoder is not yet initialized
      mCallback->InputExhausted();
      return;
    }


That way the sample, which has been queued to mMediaRawSamples will be handled when the initialisation complete.

problem solved :)
Attachment #8809271 - Flags: review?(jyavenard) → review-
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91822

BTW, while there is a bug as you've noticed. In practice it doesn't matter as all the current MediaDataDecoder::Init() call are actually synchronous. So in effect, we are actually feeding the decoder only after it's been initialised. That the data is fed twice for annexB however is a problem and is a regression introduced by bug 1297311. That should be fixed
Blocks: 1297311
Keywords: regression
Actually, it is also not a problem, because the H264Converter is always destroyed whenever a new init segment has been seen by the MediaFormatReader.

Still a coding regression
Summary: Make sure feeding the sample to the decoder after the decoder have been initialized. → Data sample fed potentially fed twice to the decoder
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91840

Thanks for your guidance. 

I just found 
http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp#302

Does this "Init" function a real async call and be used in current product?

::: dom/media/platforms/wrappers/H264Converter.cpp:257
(Diff revision 2)
>      }
> +    if (!mNeedAVCC &&
> +        !mp4_demuxer::AnnexB::ConvertSampleToAnnexB(sample, mNeedKeyframe)) {
> +      mCallback->Error(MediaResult(NS_ERROR_OUT_OF_MEMORY,
> +                                   RESULT_DETAIL("ConvertSampleToAnnexB")));
> +    }

Do you think we should do the conversion here?  Or it is not neccessary.
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91848

::: dom/media/platforms/wrappers/H264Converter.cpp:257
(Diff revision 2)
>      }
> +    if (!mNeedAVCC &&
> +        !mp4_demuxer::AnnexB::ConvertSampleToAnnexB(sample, mNeedKeyframe)) {
> +      mCallback->Error(MediaResult(NS_ERROR_OUT_OF_MEMORY,
> +                                   RESULT_DETAIL("ConvertSampleToAnnexB")));
> +    }

YES!

good call!

Don't forget to return early though. we have an OOM error
Attachment #8809271 - Flags: review?(jyavenard) → review+
Keywords: checkin-needed
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91866

::: dom/media/platforms/wrappers/H264Converter.cpp:236
(Diff revision 3)
>  
>      mInitPromiseRequest.Begin(mDecoder->Init()
>        ->Then(AbstractThread::GetCurrent()->AsTaskQueue(), __func__, this,
>               &H264Converter::OnDecoderInitDone,
>               &H264Converter::OnDecoderInitFailed));
>    }

the return should have been here !
otherwise error will always be skipped now
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d32e0a4ff68c
Data sample fed potentially fed twice to the decoder. r=jya
Keywords: checkin-needed
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91868

also the commit message needs to be fixed

"Correct data sample being fed twice to the decoder"
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

I press the "reopen for review" but nothing happened....

Need to use bugzilla to r? again.

Thanks.
Attachment #8809271 - Flags: review+ → review?(jyavenard)
Duplicate of this bug: 1316550
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91872

::: dom/media/platforms/wrappers/H264Converter.cpp:90
(Diff revision 4)
>        return;
>      }
>    } else {
>      rv = CheckForSPSChange(aSample);
> +    if (rv == NS_ERROR_NOT_INITIALIZED) {
> +      // Decoder has not yet initialized.

"Decoder has not yet been initialized"

though seeing that the return value is super explict, we may get rid of the comment or put something with more details, like

The decoder is pending initialization etc...
Attachment #8809271 - Flags: review?(jyavenard) → review+
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/324fdf1165e8
Backed out changeset d32e0a4ff68c on request from jya
Keywords: checkin-needed
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91888

::: dom/media/platforms/wrappers/H264Converter.cpp:258
(Diff revision 5)
>      }
> +    if (!mNeedAVCC &&
> +        !mp4_demuxer::AnnexB::ConvertSampleToAnnexB(sample, mNeedKeyframe)) {
> +      mCallback->Error(MediaResult(NS_ERROR_OUT_OF_MEMORY,
> +                                   RESULT_DETAIL("ConvertSampleToAnnexB")));
> +      return;

while it likely doesn't matter.

We'll probably want to clean the queued sample there for cleanliness.
Addressed comment 20.
Keywords: checkin-needed
(In reply to James Cheng[:JamesCheng] from comment #21)
> Addressed comment 20.

where?
Sorry , I don't know why my mozreview push is pushed but shows it is a draft in reviewboard and I've never met this before. And I accidently press the "discard " button by my cellphone... I will push the patch again when I'm back home.
ensuring I addressed comment 20.
Keywords: checkin-needed
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/361dfd9071b6
Correct data sample being fed twice to the decoder r=jya
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/361dfd9071b6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.