Closed Bug 1248154 Opened 4 years ago Closed 4 years ago

HTMLVideoElement.videoWidth and videoHeight don't pick up on changes.

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: jib, Assigned: pehrsons)

Details

Attachments

(1 file)

Using the patches and fiddle in Bug 1244913 comment 5, I observe that without a 1 second delay, I get the wrong results from videoWidth and videoHeight.

I.e. I have to do this:

  var dimensions = v => v.videoWidth +"x"+ v.videoHeight;

  var set = (v, vinfo, stream) => Promise.resolve(v.srcObject = stream)
    .then(() => new Promise(r => v.onloadedmetadata = r))
    .then(() => wait(1000)) // Bug!!
    .then(() => setInterval(() => update(vinfo, dimensions(v), 1000)))
    .catch(log);

The reason the video changes has to do with delays in the WebRTC stack wrt scaling, which are perhaps unfortunate, but the fact that these values don't reflect changes correctly aren't helping, and should probably be fixed regardless of that.
Note that I'm using setInterval, which means that repeated prodding keeps producing the wrong value after the change. I.e. the problem does not self-correct, which is unfortunate for diagnostics.
jya - any ideas?  I'm pretty sure this used to work
Component: Audio/Video → Audio/Video: Playback
Flags: needinfo?(jyavenard)
Could it be bug 1237809? that's the only changes I can think off that changed the handling of dimensions.
But it should be the opposite of what is seeing and was rather specific to plain media playback.

I don't know how webrtc handles changes of dimensions, the whole thing in HTMLMediaElement when it comes to media stream has always looked highly suspicious to me.
Flags: needinfo?(jyavenard)
I tend to suspect changes to when/how onloadedmetadata fires instead of that.

Pehrsons - Could it be related to bug 1240478?
Flags: needinfo?(pehrsons)
(In reply to Randell Jesup [:jesup] from comment #4)
> I tend to suspect changes to when/how onloadedmetadata fires instead of that.
> 
> Pehrsons - Could it be related to bug 1240478?

Maybe.

I thought the size would be updated by the ImageContainer independent of what source we are using. Is that not happening?
Flags: needinfo?(pehrsons)
We can easily write a better test for resize and videoWidth/Height now that we can get a stream from canvas.
It works fine here: https://jsfiddle.net/pehrsons/k1cnz9rw/

jib, can you provide a more complete example?
Flags: needinfo?(jib)
Comment out the wait(1000) line in this fiddle: https://jsfiddle.net/kv14z7r6/
Flags: needinfo?(jib)
I forked that fiddle and added some console logs (e.g., onresize so we see what's really going on): https://jsfiddle.net/pehrsons/7L9k7zLq/

Also your setInterval call was broken - the resolution now gets fixed rather quickly.

It seems however like a receiving MediaPipeline creates a 640x480 frame initially. We should figure out why.
I traced the 640x480 back to version 31: https://hg.mozilla.org/mozilla-central/rev/8733191b0d10

Not sure if the bug is exposed there. The way they get through now is by MediaPipelineReceiveVideo::Init calling WebrtcVideoConduit::AttachRenderer which calls back to the new renderer (forwarded to the pipeline) using the method FrameSizeChanged() with the default args 640x480, since no real frames had been received and decoded yet.

If the default is 0x0 instead, I think it'll work as intended. Also seems more semantically correct...
I can also note btw, that we do pass a couple of null images through as frames with size 640x480. This because the MediaPipelineReceiveVideo::PipelineListener (a MediaStreamListener) is pull based.
Works for me. jib, want to double check?
Assignee: nobody → pehrsons
Status: NEW → ASSIGNED
Flags: needinfo?(jib)
Attachment #8727460 - Flags: review?(pkerr)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #9)
> Also your setInterval call was broken

Doh!

> Works for me. jib, want to double check?

Wfm too, even without the setInterval.
Flags: needinfo?(jib)
Attachment #8727460 - Flags: review?(pkerr) → review+
Looks good to go. Anything else needed for this to land?
https://hg.mozilla.org/mozilla-central/rev/5213de555ac0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.