Closed Bug 1344649 Opened 7 years ago Closed 7 years ago

Remove MediaDataDecoder::ConfigurationChanged API

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: jya, Assigned: jhlin)

References

Details

Attachments

(5 files, 4 obsolete files)

Bug 1344649 - part 2: let VideoData::CreateFromImage() accept only neccessary parameters.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1344649 - part 3: rename DurationMap and turn it into a generic class.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1344649 - part 4: store frame sizes in queue rather than relying on ConfigurationChanged().
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1344649 - part 5: deprecate ConfigurationChanged() once again.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1344649 - part 1: use picture instead of display size to construct Image.
59 bytes, text/x-review-board-request
jya
: review+
gchang
: approval-mozilla-aurora+
Details 
As discussed with :jolin

Rather than having an extra API that calls a particular entry (ConfigurationChanged) whenever a change of resolution is detected.

We should instead simply add to the MediaRawData the related VideoInfo/AudioInfo in a TrackInfoSharedPtr (or SharedTrackInfo in 53 and earlier).

This will make the change easier to adapt in order to uplift bug 1336431
Assignee: nobody → jolin
Blocks: 1343847
Comment on attachment 8843901 [details]
Bug 1344649 - part 1: let VideoData::CreateFromImage() accept only neccessary parameters.

https://reviewboard.mozilla.org/r/117520/#review119146

you can't commit this change as-is.. it will fail the mochitests.
Attachment #8843901 - Flags: review?(jyavenard) → review+
Comment on attachment 8843902 [details]
Bug 1344649 - part 2: rename DurationMap and turn it into a generic class.

https://reviewboard.mozilla.org/r/117522/#review119148

I don't think this should be fully reverted.

all we need instead is change the MFR to stop calling ConfigurationChanged() and the H264Converter to simply create a new SharedTrackInfo whenever a change of resolution is detected.

I'm expecting to see most if this back in a follow up patch.

The commit title is also incomplete as there's more revert that Bug 1336431 P5... there's bug 1342217 and some of bug 1339748

::: dom/media/MediaFormatReader.cpp:1963
(Diff revision 1)
>    while (decoder.mQueuedSamples.Length()) {
>      RefPtr<MediaRawData> sample = decoder.mQueuedSamples[0];
>      RefPtr<TrackInfoSharedPtr> info = sample->mTrackInfo;
>  
>      if (info && decoder.mLastStreamSourceID != info->GetID()) {
> -      if (decoder.mNextStreamSourceID.isNothing()
> +      bool supportRecycling = MediaPrefs::MediaDecoderCheckRecycling()

I think this should stay as-is, it doesn't need to be reverted

::: dom/media/MediaFormatReader.cpp
(Diff revision 1)
>          nsTArray<RefPtr<MediaRawData>> samples{ Move(decoder.mQueuedSamples) };
>          ShutdownDecoder(aTrack);
>          if (sample->mKeyframe) {
>            decoder.mQueuedSamples.AppendElements(Move(samples));
>          }
> -      } else if (decoder.mInfo && *decoder.mInfo != *info) {

only that part needs to be removed

::: dom/media/platforms/wrappers/H264Converter.cpp:291
(Diff revision 1)
>        MediaResult(NS_ERROR_OUT_OF_MEMORY,
>                    RESULT_DETAIL("ConvertSampleToAnnexB")),
>        __func__);
>      return;
>    }
> -  if (CanRecycleDecoder()) {
> +

that should be replaced by creating a new TrackInfoSharedPtr that would be added to the MediaRawData.

::: dom/media/platforms/wrappers/H264Converter.cpp
(Diff revision 1)
> -
>    if (CanRecycleDecoder()) {
>      // Do not recreate the decoder, reuse it.
>      UpdateConfigFromExtraData(extra_data);
> -    // Ideally we would want to drain the decoder instead of flushing it.
> -    // However the draining operation requires calling Drain and looping several

this should stay too...
Attachment #8843902 - Flags: review?(jyavenard) → review+
Priority: -- → P1
Comment on attachment 8843902 [details]
Bug 1344649 - part 2: rename DurationMap and turn it into a generic class.

https://reviewboard.mozilla.org/r/117522/#review119148

> I think this should stay as-is, it doesn't need to be reverted

Okay. Will bring in back in next version.

> that should be replaced by creating a new TrackInfoSharedPtr that would be added to the MediaRawData.

Yes, I was going to do this in another bug/patch, but you're right that it makes more sense being here.

> this should stay too...

Sorry but I don't quite understand why draining/flushing is still needed here after we attach updated video info copy in `mPendingSample`. I thought that is for dropping outputs before configuration change so they won't incorrectly use the new dimensions. Could you please explain it? Thanks.
Flags: needinfo?(jyavenard)
Comment on attachment 8843902 [details]
Bug 1344649 - part 2: rename DurationMap and turn it into a generic class.

https://reviewboard.mozilla.org/r/117522/#review119148

> Sorry but I don't quite understand why draining/flushing is still needed here after we attach updated video info copy in `mPendingSample`. I thought that is for dropping outputs before configuration change so they won't incorrectly use the new dimensions. Could you please explain it? Thanks.

Because I see two parts of this bug.

The reversal of ConfigurationChange is one thing, and that's to be replaced by instead setting the TrackInfoSharedPtr to the current configuration.

And then you have what's causing the problem of bug 1343847: that's the drain/flush. For this part, we need to figure out why the draining cause the problem (it shouldn't really) and either fix that, or get around the problem and not drain.

Separating the two tasks is better, and it doesn't make incorrectly blame the problem to using the ConfigurationChanged API, they are two separate issues.

It sounds much nicer to me to do it that way.
Flags: needinfo?(jyavenard)
Good point. Thanks!
I did a complete rewrite to address comments. Hope this version make more sense now.
Attachment #8843901 - Attachment is obsolete: true
Attachment #8843902 - Attachment is obsolete: true
Attachment #8844756 - Attachment is obsolete: true
Attachment #8844756 - Flags: review?(jyavenard)
Attachment #8844757 - Attachment is obsolete: true
Attachment #8844757 - Flags: review?(jyavenard)
Comment on attachment 8844759 [details]
Bug 1344649 - part 2: let VideoData::CreateFromImage() accept only neccessary parameters.

https://reviewboard.mozilla.org/r/118074/#review120104

Personally, I think it would be easier to continue passing a VideoInfo argument, it's less changes and makes it easier to understand (I think)

::: commit-message-2cce2:3
(Diff revision 1)
> +Bug 1344649 - part 1: let VideoData::CreateFromImage() accept only neccessary parameters. r?jya
> +
> +VideoData doesn't care what's in aInfo but display size, and aPicture is unused.

display size and aPicture are unused

::: dom/media/platforms/android/RemoteDataDecoder.cpp:185
(Diff revision 1)
>  
>        if (size > 0) {
>          MutexAutoLock lock(mDecoder->mMutex);
>  
>          RefPtr<layers::Image> img = new SurfaceTextureImage(
> -          mDecoder->mSurfaceTexture.get(), mDecoder->mConfig.mDisplay,
> +          mDecoder->mSurfaceTexture.get(), mDecoder->mConfig.mImage,

that can't be good.

This should be done in another patch as it's out of scope with the rest of the changes.

::: dom/media/platforms/apple/AppleVTDecoder.cpp:355
(Diff revision 1)
>    // Where our resulting image will end up.
>    RefPtr<MediaData> data;
>    // Bounds.
>    VideoInfo info;
>    info.mDisplay = nsIntSize(mDisplayWidth, mDisplayHeight);
>    gfx::IntRect visible = gfx::IntRect(0,

visible scope should be moved from inside if (mUseSoftwareImages) { as it's no longer used otherwise
Attachment #8844759 - Flags: review?(jyavenard) → review+
Comment on attachment 8844760 [details]
Bug 1344649 - part 3: rename DurationMap and turn it into a generic class.

https://reviewboard.mozilla.org/r/118076/#review120110

not fan of the name but hey .... SimpleMap?
Attachment #8844760 - Flags: review?(jyavenard) → review+
Comment on attachment 8844761 [details]
Bug 1344649 - part 4: store frame sizes in queue rather than relying on ConfigurationChanged().

https://reviewboard.mozilla.org/r/118078/#review120116

::: dom/media/platforms/android/RemoteDataDecoder.cpp:140
(Diff revision 1)
>      java::CodecProxy::GlobalRef mCodec;
>      java::Sample::GlobalRef mSample;
>    };
>  
> +
> +  class InputInfo {

bracket on next line:
class InputInfo
{

::: dom/media/platforms/android/RemoteDataDecoder.cpp:142
(Diff revision 1)
>    };
>  
> +
> +  class InputInfo {
> +  public:
> +    InputInfo()

InputInfo() { } 

(note the space between the brackets)
Attachment #8844761 - Flags: review?(jyavenard) → review+
Comment on attachment 8844762 [details]
Bug 1344649 - part 5: deprecate ConfigurationChanged() once again.

https://reviewboard.mozilla.org/r/118080/#review120132

::: dom/media/MediaFormatReader.cpp
(Diff revision 1)
>          nsTArray<RefPtr<MediaRawData>> samples{ Move(decoder.mQueuedSamples) };
>          ShutdownDecoder(aTrack);
>          if (sample->mKeyframe) {
>            decoder.mQueuedSamples.AppendElements(Move(samples));
>          }
> -      } else if (decoder.mInfo && *decoder.mInfo != *info) {

You need to set the MediaRawData with the info...
Attachment #8844762 - Flags: review?(jyavenard) → review-
Comment on attachment 8844761 [details]
Bug 1344649 - part 4: store frame sizes in queue rather than relying on ConfigurationChanged().

https://reviewboard.mozilla.org/r/118078/#review120134

::: dom/media/platforms/wrappers/H264Converter.cpp:326
(Diff revision 1)
>    RefPtr<MediaRawData> sample = aSample;
>  
>    if (CanRecycleDecoder()) {
>      // Do not recreate the decoder, reuse it.
>      UpdateConfigFromExtraData(extra_data);
> +    sample->mTrackInfo = new TrackInfoSharedPtr(mCurrentConfig, 0);

This should be done in part 4...

Additionally, you should check if a sample->mTrackInfo is already set or not, as if it is, it means you're using MSE and the MediaRawData already contains the current value.
Comment on attachment 8844762 [details]
Bug 1344649 - part 5: deprecate ConfigurationChanged() once again.

https://reviewboard.mozilla.org/r/118080/#review120166
Attachment #8844762 - Flags: review- → review+
Comment on attachment 8845261 [details]
Bug 1344649 - part 1: use picture instead of display size to construct Image.

https://reviewboard.mozilla.org/r/118432/#review120374
Attachment #8845261 - Flags: review?(jyavenard) → review+
Comment on attachment 8844762 [details]
Bug 1344649 - part 5: deprecate ConfigurationChanged() once again.

https://reviewboard.mozilla.org/r/118080/#review120376

::: dom/media/platforms/wrappers/H264Converter.h
(Diff revision 2)
>    }
>  
>    void DecodeFirstSample(MediaRawData* aSample);
>  
>    RefPtr<PlatformDecoderModule> mPDM;
> -  const VideoInfo mOriginalConfig;

I think all related to mOriginalConfig should stay, the logic was wrong before..

::: dom/media/platforms/wrappers/H264Converter.cpp:206
(Diff revision 2)
>      mLastError = NS_ERROR_FAILURE;
>      return NS_ERROR_FAILURE;
>    }
>  
>    mDecoder = mPDM->CreateVideoDecoder({
> -    mUseOriginalConfig ? mOriginalConfig : mCurrentConfig,
> +    mCurrentConfig,

dont revert that code, otherwise the VideoInfo used to create the decoder will be chsnged in a non threadsafe fashion
Comment on attachment 8844762 [details]
Bug 1344649 - part 5: deprecate ConfigurationChanged() once again.

https://reviewboard.mozilla.org/r/118080/#review120132

> You need to set the MediaRawData with the info...

doh...
sorry for that.
See Also: → 1345768
No longer blocks: 1343847
Pushed by jolin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2bf3ae75d54
part 1: use picture instead of display size to construct Image. r=jya
https://hg.mozilla.org/integration/autoland/rev/5eb2d62d3c61
part 2: let VideoData::CreateFromImage() accept only neccessary parameters. r=jya
https://hg.mozilla.org/integration/autoland/rev/71954a8ab450
part 3: rename DurationMap and turn it into a generic class. r=jya
https://hg.mozilla.org/integration/autoland/rev/ba81bdbba7e4
part 4: store frame sizes in queue rather than relying on ConfigurationChanged(). r=jya
https://hg.mozilla.org/integration/autoland/rev/fcd6c84fa5b0
part 5: deprecate ConfigurationChanged() once again. r=jya
Comment on attachment 8845261 [details]
Bug 1344649 - part 1: use picture instead of display size to construct Image.

Please approve uplifting all patches to aurora. Thanks a lot. 

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: wrong video size/aspect ratio
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]:low risk
[Why is the change risky/not risky?]: it changes only video frames size.
[String changes made/needed]:none
Attachment #8845261 - Flags: approval-mozilla-aurora?
Fails to merge:
    Merge failed for commit fcd6c84fa5b0 (parent fd8bdabb4813)

    grafting 386116:fcd6c84fa5b0 "Bug 1344649 - part 5: deprecate ConfigurationChanged() once again. r=jya"
    merging dom/media/MediaFormatReader.cpp
    merging dom/media/platforms/PlatformDecoderModule.h
    merging dom/media/platforms/wrappers/H264Converter.cpp
    merging dom/media/platforms/wrappers/H264Converter.h
    warning: conflicts while merging dom/media/platforms/wrappers/H264Converter.cpp! (edit, then use 'hg resolve --mark')
    abort: unresolved conflicts, can't continue
    (use 'hg resolve' and 'hg graft --continue')

    Merge failed for commit ba81bdbba7e4 (parent fd8bdabb4813)

    grafting 386115:ba81bdbba7e4 "Bug 1344649 - part 4: store frame sizes in queue rather than relying on ConfigurationChanged(). r=jya"
    merging dom/media/platforms/android/RemoteDataDecoder.cpp
    warning: conflicts while merging dom/media/platforms/android/RemoteDataDecoder.cpp! (edit, then use 'hg resolve --mark')
    abort: unresolved conflicts, can't continue
    (use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(jolin)
I'm terribly sorry. These patches are dependent on bug 1344661 and bug 1345545 and I forgot to list them as other uplifts needed. Please help uplift them too. Thanks a lot and sorry again.
Flags: needinfo?(jolin)
Hi John,
Are there any ways that QA can help verify this? These patches are huge to me and your risk adjustment doesn't seem correct to me. I will hold this until we have better verification steps for QA to test.
Flags: needinfo?(jolin)
Hi Gary,
 These patches are actually not as huge as it looks like. This bug rewrites bug 1336431 to make it easier to uplift to beta(53) and a large part of the changeset is reverting that. There are only 3 real logic changes which sets (correct) video frame sizes.
 
 As for QA test, I'm afraid that I haven't met the symptom in the field in person and don't have a SOP to reproduce it. But the old logic, as Jean-Yves said in bug 1336431, is just plain wrong.
 
 Also, the work of bug 1345599 was developed on top of patches here so it will be more difficult to uplift that without this.

 Please let me know if you have more concerns. Thanks a lot.
Flags: needinfo?(jolin) → needinfo?(gchang)
Oops! Just found that I misspell your name. Terribly sorry about that, Gerry.
Comment on attachment 8845261 [details]
Bug 1344649 - part 1: use picture instead of display size to construct Image.

This is much more clear to me. Thanks for the explanation. Aurora54+.
Flags: needinfo?(gchang)
Attachment #8845261 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: